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 <[email protected]> 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 [email protected] http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface
