Clickable links

[detrout]

This implements a mechanism for selecting hyperlinks in the rich text editor.

It's using QtGui.QTextEdit's anchorAt to determine the existence of a link, so links that are printed without being wrapped in aren't going to be detected.

The implementation conditionally adds "Open Link" and "Copy Link Anchor" as inspired by gnome-terminal. (I didn't spend very much time thinking about the order of menu options)

A typical browser security feature is to show the user the target of a link before they can actually select it, I implemented that by adding a tooltop when the mouse hovers over an anchor.

That was implemented by turning on mouseMoveEvent messages on the rich QTextEdit controland checking to see if an anchor is under the current mouse position. If it is and its a different anchor than what we're currently displaying (or not displaying) it'll pop up the tooltip.

The advantage to storing the displayed anchor in ConsoleWidget._anchor allows me to be sure that the thing the user is opening was displayed to to them, and to prevent the tooltip from constantly getting redrawn as you move the mouse around the link.

The plain text control QPlainTextEdit does have the anchorAt method, but I was imagining lacked any way to receive html text so I didn't bother turning on the mouse move tracking for it.

[bfroehle]

I don't think we want to completely override the moveMouseEvent... just add this one feature to it. In particular I can no longer select text with the mouse after applying this patch.

Otherwise things look pretty good to me!

[bfroehle]

As a start, I think you can probably move the logic to eventFilter (test that etype == QtCore.QEvent.MouseMove). Also the coordinates should probably be passed through _control.mapToGlobal before being passed to the QToolTip.

[detrout]

Ok, I wasn't sure if the the eventFilter was for handling events or if it was just controlling what reached the control. I'll try moving it tonight.

[detrout]

New patch committed to clickable-links. Do I need to resubmit the pull request, do you get notifications on new pushes?

[takluyver]

@detrout : No need to resubmit, but we don't automatically get notified, so it's useful to comment - as you did - so we know it's been updated.

[detrout]

I forgot to update my test case for the mouse hover code.

Hopefully the text never ends up 50 mouse units high.

[takluyver]

Am I right that this is currently waiting for review from people who know about Qt?

[detrout]

I'm not aware of anything else I need to do for this patch.

Potential future work would be adding support for detecting plain text URLs and converting them to links like recently added to the HTML notebook, but I could see wanting to change the default qtconsol formatting as well.

[takluyver]

Thanks @detrout

Pinging @bfroehle @epatters @minrk to get this reviewed - please ping anyone else who you think might be able to review it. It looks OK to me, but I'm not so familiar with Qt.

[bfroehle]

I'm not quite familiar enough with Qt to give a final okay here. Also I have some comments which I'll post in the diff view.

[bfroehle]

Same as if not anchor?

[detrout]

I bet it'd work, I'm just not used to the "if not anchor:" idiom so tend to be uncertain if the empty string is false.

[bfroehle]

Also is there any reason to abstract the hide/show tooltip into a signal/slot framework?

[detrout]

Test temporary variable anchor to see if the pointer moved over a different anchor.

[detrout]

Why anchor temporary variable

The reason for storing the result of "anchor = self._control.anchorAt(event.pos())" as a temporary variable instead of directly into self._anchor is I wanted to check to see if the pointer was still over the same anchor (line 443).

I'd seen some applications where the tooltip keeps getting redrawn in slightly different locations as the pointer moves and it's quite annoying. This way the tooltip gets drawn once until the anchorAt is empty or is over a different anchor.

I was assuming it's possible for two anchors to be next to each other without receiving a mouse move message that would clear self._anchor.

Performance of calling self._control.anchorAt

As for performance. I'm basically depending on Qt to batch the mouse move messages to keep the lookup call count low.

The other solution I'd thought of was to have receiving a mouse move reset and then start a short timer, and only do the self._control.anchorAt call once the pointer hadn't moved for 200ms.

Then I decided I wasn't sure where to put the timer class and went for the simpler to read solution.

I'm not sure of a good way to test which is lighter-weight between the timer and just calling anchorAt.

[bfroehle]

Yes, okay. I don't think performance is an issue here.

I'm a little worried about the fragility of self._anchor. Either we need to put a comment in eventFilter noting that self._anchor is used by other methods or we need an approach which skips it.

I've found the following to work well:

In eventFilter:

        elif etype == QtCore.QEvent.MouseMove:
            anchor = self._control.anchorAt(event.pos())
            QtGui.QToolTip.showText(event.globalPos(), anchor, self._control)

In _context_menu_make:

        anchor = self._control.anchorAt(pos)
        if anchor:
            menu.addSeparator()
            self.copy_link_action = menu.addAction(
                'Copy Link Address', lambda: self.copy_anchor(anchor=anchor))
            self.open_link_action = menu.addAction(
                'Open Link', lambda: self.open_anchor(anchor=anchor))

    def copy_anchor(self, anchor):
        """Copy anchor text to the clipboard."""
        QtGui.QApplication.clipboard().setText(anchor)

    def open_anchor(self, anchor):
        """Open selected anchor in the default webbrowser."""
        webbrowser.open(anchor)

[bfroehle]

Alternatively, if we don't like tooltips we could use a status bar:

        elif etype == QtCore.QEvent.MouseMove:
            anchor = self._control.anchorAt(event.pos())
            tip = QtGui.QStatusTipEvent(anchor)
            QtGui.QApplication.sendEvent(self._control, tip)

although this requires creating a QStatusBar by calling self.statusBar() in MainWindow.__init__.

[detrout]

I can see wanting to remove the added class variable, but I do want to protect against redrawing, though if using the status bar redrawing would probably be safe, since it avoids having the text drift around the screen as .pos changes.

I can see two possibilities to prevent unnecessary QToolTip.showText calls in eventFilter

    elif etype == QtCore.QEvent.MouseMove:
        anchor = self._control.anchorAt(event.pos())
        if not anchor:
           QtGui.QToolTip.hideText()
        elif anchor not in QtGui.QToolTip.text():
           QtGui.QToolTip.showText(event.globalPos(), anchor)

That would drop self._anchor and use the QToolTip text as the temporary variable for checking to see if we drifted over a different link.

(Also your choice between anchor not in QToolTip.text() or anchor != QToolTip.text() with the implementation as it currently stands they're equivalent, however the not in would still work if someone added more formatting to the tooltip.)

The other possibility would be:

    elif etype == QtCore.QEvent.MouseMove:
        anchor = self._control.anchorAt(event.pos())
        if not anchor:
           QtGui.QToolTip.hideText()
        elif not QtGui.QToolTip.isVisible():
           QtGui.QToolTip.showText(event.globalPos(), anchor)

That should remove a tooltip if there's no anchor, show a tool tip if there isn't currently one, and do nothing if there is a tooltip and an anchor. However it can't tell if the user moved over a different anchor.

For either of those implementations, I'd use the lambda function implementation you described for _context_menu_make.

[bfroehle]

If you read the documentation for QToolTip, you'll see that it already protects against redrawing. In addition, passing the empty string is equivalent to calling hideText.

[detrout]

That's remarkably convenient of them. Thank you for calling that part of the documentation out.

Fixing up patch as you described, I should have it posted in a few minutes.

[detrout]

I think everything's been updated. Anything else I should do?

[bfroehle]

Ahh, thanks for the update. Unfortunately GitHub doesn't send emails when you just push some changes. :/ Merging now.

[detrout]

Good to know that I should poke the pull request when finishing up the patch.

(Whoohoo! I successfully contributed code!)

[bfroehle]

@detrout Thanks again. A nice improvement, I think. 👍