On Tue, Oct 13, 2015 at 7:32 AM, Dirk Hohndel <d...@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 _______________________________________________ subsurface mailing list subsurface@subsurface-divelog.org http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface