On Mon, Oct 12, 2015 at 10:08:35PM -0700, K. "pestophagous" Heller wrote:
> In the spirit of "Do the simplest thing that could
> possibly work": capture Ctrl+leftclick mouse events
> in the Notes area. If the string under the clicked
> position is a valid url, then launch it.
>
> Many common URI schemes will work. Typing a url that
> starts with https:// will work. So will mailto: and
> file://
>
> See #733
Neat. I have a few concerns with the code, but the idea itself is a cool
addition. Obviously post-4.5 :-)
> diff --git a/qt-ui/maintab.cpp b/qt-ui/maintab.cpp
> index e97812c..35cda6e 100644
> --- a/qt-ui/maintab.cpp
> +++ b/qt-ui/maintab.cpp
> @@ -202,6 +202,11 @@ MainTab::MainTab(QWidget *parent) : QTabWidget(parent),
> connect(ui.diveNotesMessage, &KMessageWidget::showAnimationFinished,
> ui.location,
> &DiveLocationLineEdit::fixPopupPosition);
>
> + //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?
> diff --git a/qt-ui/simplewidgets.cpp b/qt-ui/simplewidgets.cpp
> index 57fc56b..e16d05f 100644
> --- a/qt-ui/simplewidgets.cpp
> +++ b/qt-ui/simplewidgets.cpp
> @@ -735,3 +737,137 @@ void MultiFilter::closeFilter()
> MultiFilterSortModel::instance()->clearFilter();
> hide();
> }
> +
> +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...)
> +{
> + // 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?
> + const bool isCtrlClick = evt->type() == QEvent::MouseButtonPress &&
> + 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?
> + return false; // (slight lie). indicate that we didn't do anything with
> the event.
Why?
> +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.
> +void TextHyperlinkEventFilter::handleUrlTooltip(const QString &urlStr, const
> QPoint &pos)
> +{
> + if (urlStr.isEmpty()) {
> + QToolTip::hideText();
> + } else {
> + 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?
> +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 result;
> + QString grownText;
> + QString noSpaces;
> + bool movedOk = false;
> +
> + do {
> + result = grownText; // this is a no-op on the first visit.
> +
> + if (searchBackwards) {
> + movedOk =
> cursor->movePosition(QTextCursor::PreviousWord, QTextCursor::KeepAnchor);
> + } else {
> + movedOk = cursor->movePosition(QTextCursor::NextWord,
> QTextCursor::KeepAnchor);
> + }
> +
> + grownText = cursor->selectedText();
> + noSpaces = grownText.simplified().replace(" ", "");
> + } while (grownText == noSpaces && movedOk);
> +
> + // while growing the selection forwards, we have an extra step to do:
> + if (!searchBackwards) {
> + /*
> + The cursor keeps jumping to the start of the next word.
> + (for example) in the string "mn.abcd.edu is the spot" you land at
> + m,a,e,i (the 'i' in 'is). if we stop at e, then we only capture
> + "mn.abcd." for the url (wrong). So we have to go to 'i', to
> + capture "mn.abcd.edu " (with trailing space), and then clean it up.
> + */
This looks whitespace damaged
> + QStringList list = grownText.split(QRegExp("\\s"),
> QString::SkipEmptyParts);
> + if (!list.isEmpty()) {
> + result = list[0];
> + }
> + }
> +
> + return result;
> +}
> +
> +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.
> +
> + cursor->select(QTextCursor::WordUnderCursor);
> + QString maybeUrlStr = cursor->selectedText();
> +
> + const bool soFarSoGood = !maybeUrlStr.simplified().replace(" ",
> "").isEmpty();
> +
> + if (soFarSoGood && !stringMeetsOurUrlRequirements(maybeUrlStr)) {
> + // 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*/);
> + QString right = fromCursorTilWhitespace(&cursor2, false);
> + maybeUrlStr = left + right;
> + }
OK, I think I now know what the function above is needed for - it still
deserves a bit more commentary
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.
Thanks
/D
_______________________________________________
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface
