[PATCH] Parse html links in the Notes section.

[Dirk Hohndel]

Just a quick reminder... you wanted to resend this one after fixing the
issues I pointed out below...

/D

On Tue, Oct 13, 2015 at 04:01:55PM -0700, K. "pestophagous" Heller wrote:
> 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