[PATCH] Parse html links in the Notes section.

K. "pestophagous" Heller pestophagous at gmail.com
Tue Oct 13 16:01:55 PDT 2015


On Tue, Oct 13, 2015 at 7:32 AM, Dirk Hohndel <dirk at hohndel.org> wrote:
>
> Neat. I have a few concerns with the code, but the idea itself is a cool
> addition. Obviously post-4.5 :-)

Yes :)

I half-expected to not see any reply to this until after 4.5. I do not
want to be a distraction from that release.


>> +     //if (true).  // to make URL clickability optional, simply branch here based on a preference.
>
> So why would this be optional? In general I aim for fewer options, not
> more, but maybe I'm missing something?
>

no reason. up to your (and the groups') discretion. also i wanted to
highlight that only one line of any existing class was "harmed" in the
making of this feature. (offering some assurance that i didn't break
any existing functionality.)

>> +
>> +TextHyperlinkEventFilter::TextHyperlinkEventFilter(QTextEdit *txtEdit) : QObject(txtEdit),
>> +                                                                      textEdit(txtEdit),
>> +                                                                      scrollView(textEdit->viewport())
>
> That's not our typical indentation scheme (admittedly we aren't super
> consistent about it, but still...)
>

darn. i ran that through the whitespace.pl script. nonetheless, i
should have still compared it to the "C++ constructor initialization
list" notes in CodingStyle. i will clean up my initialization list,
and i will perhaps see about making whitespace.pl be better about
doing my dirtywork in the future.


>> +{
>> +     // lesson learned. install filter on viewport:
>> +     // http://stackoverflow.com/questions/31581453/qplaintextedit-double-click-event
>
> This comment is very much about your process of writing code... I haven't
> followed the link, but could you instead summarize WHY this is the way to
> go in the comment?

thanks. will do.


>
>> +     return false; // (slight lie). indicate that we didn't do anything with the event.
>
> Why?

in working with other GUI frameworks, i have been well-served by a
philosophy of "always allow the event/signal to bubble up (continue
propagating through windows and parents) unless i have a specific
reason to block it."  If i implement some custom reaction to an event,
i err on the side of caution by also trying to allow all default
event-handling to still take place.  Unless i know that my "reaction
code" really should be mutually exclusive with whatever the default
behaviors are.

ugh. that's too wordy. Did i make any sense? The only options are to
return true (letting Qt think that event-handling is finished for this
event) or return false (to let Qt proceed in case Qt has more
activities planned for this event). I chose false. I think it is "a
safe bet", but i am no Qt expert.

>
>> +void TextHyperlinkEventFilter::handleUrlClick(const QString &urlStr)
>> +{
>> +     if (!urlStr.isEmpty()) {
>> +             QUrl url(urlStr, QUrl::StrictMode);
>> +             QDesktopServices::openUrl(url);
>
> Are there any security implications to keep in mind here? I haven't
> studied what openURL() is capable of doing, just asking.
>

That crossed my mind. I have not studied the matter.  That is part of
why I bumped out "stringMeetsOurUrlRequirements" into a separate
function.  If any added security screening was deemed necessary, I
figured it could go in there.

>> +                              static_cast<QMouseEvent *>(evt)->modifiers() & Qt::ControlModifier &&
>> +                              static_cast<QMouseEvent *>(evt)->button() == Qt::LeftButton;
>
> Silly question - how does this translate on the Mac? Is it actually the
> key labled "Ctrl" on all platforms or is this the command key on the Mac?

>> +             QToolTip::showText(pos, tr("Ctrl+click to visit %1").arg(urlStr));
>
> And this is why I asked about "Ctrl" above - is this tooltip correct on
> the Mac?
>

You caught me!  To your first "silly question" i was going to proudly
state that I had indeed investigated that already.  The
Qt::ControlModifier is seamlessly reinterpreted by Qt-for-Mac as the
Command key.

However... even after I took the time to so diligently verify that, I
still flubbed the tooltip. Ha. I will look for something like a const
string "Qt:Name_of_Ctrl_Key" and use that.  Qt must almost certainly
have such a string that I can use.


>> +QString TextHyperlinkEventFilter::fromCursorTilWhitespace(QTextCursor *cursor, const bool searchBackwards)
>
> Ok, first function where I'm not quite sure what it does and why it does
> it. Could you add a brief comment?

>> +
>> +QString TextHyperlinkEventFilter::tryToFormulateUrl(QTextCursor *cursor)
>> +{
>> +     // If instead of (or in addition to) QTextCursor::WordUnderCursor we could
>> +     // also have some Qt feature like 'unbroken-string-of-nonwhitespace-under-cursor',
>> +     // then the following logic would not be necessary to do.
>> +     // http://stackoverflow.com/questions/19262064/pyqt-qtextcursor-wordundercursor-not-working-as-expected
>
> Again, this is a comment that makes total sense when you write it, but is
> less useful when someone else is trying to read it / understand the code.
>> +
>> +             // If we don't yet have a full url, try to expand til we get one.  Note:
>> +             // after requesting WordUnderCursor, empirically (all platforms, in
>> +             // Qt5), the 'anchor' is just past the end of the word.
>> +
>> +             QTextCursor cursor2(*cursor);
>> +             QString left = fromCursorTilWhitespace(cursor, true /*searchBackwards*/);
>
> OK, I think I now know what the function above is needed for - it still
> deserves a bit more commentary

Yes. How can I best document all that?

    tryToFormulateUrl
    fromCursorTilWhitespace

tryToFormulateUrl mainly exists because WordUnderCursor will not treat
"http://m.abc.def" as a word.

Would the sentence that I just wrote (right above) be a good start to
a docu-comment for "tryToFormulateUrl" ??

fromCursorTilWhitespace calls cursor->movePosition repeatedly, while
preserving the original 'anchor' (qt terminology) of the cursor.
tryToFormulateUrl invokes fromCursorTilWhitespace two times (once with
a forward moving cursor and once in the backwards direction) in order
to expand the selection to try to capture a complete string like
"http://m.abc.def"

Is the preceding paragraph any better?

Out of the candidate pool of both the comments in the original patch
and the prose I just proposed above, I am "all ears" to know which
parts sound clearest.  To some degree, unless a person has spent hours
"cursing" the QTextCursor (heh)—and even once your have—its workings
are tricky to articulate.

> Overall a really good contribution. As you can tell, most of my comments
> are nit-picks and small stuff - I don't think there's anything
> fundamentally wrong here. But it's enough that I would like to ask you to
> resubmit this.

It would be my pleasure. I will work through the following checklist:

[_] remove the "if (true)" comment about making things preference-based.
[_] fix indentation in ctor, and also whitespace damage in comment block.
[_] replace "Ctrl" in the tooltip text with something that would say
"Cmd" on mac.
[_] find some better prose to explain the cursor comings-and-goings

[_] ... look into security implications??

Anything missing on that checklist?

/K


More information about the subsurface mailing list