[PATCH] Parse html links in the Notes section.

[Dirk Hohndel]

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