URL linkification in People (easy)

[humdinger]

This is hrev30391.

BeOS' People app had the URL label marked as a link, opening the address in the browser. Haiku's should have that too, it's a convenient way to verify an address.

[kitallis]

This sort of fixes up the verify feature of the URL. Clicking on the "URL" label text will launch the URL inside the field

[humdinger]

Cool you're working on this, kitallis[[BR]] You think you could also account for ​https:// and ​ftp:// urls?

[kitallis]

Replying to humdinger:

Cool you're working on this, kitallis[[BR]] You think you could also account for ​https:// and ​ftp:// urls?

Yeah, Sure. It'd also be nice if the Email label passes it on to the Mail app (text/x-email). But before that I'd need to 1) find a workaround for those hard-coded values 2) underline the clickable label and probably 3) make the whole source code compatible with the Haiku Code Guidelines? also 4) learn to not post on the Trac when the patch is partially ready :P

[humdinger]

So, you got your work cut out for you. Get cracking! :)

[stippi]

Hi kitallis, in your patch, you should call the inherited version of MouseDown(), if you are not handling the click. Otherwise you break ordinary text field behavior. Also the hardcoded values for the click position in MouseDown() are not the way to go. If you explain what exactly you are checking there, I could probably help you figure out the correct code.

[stippi]

Ok, just now noticed it's supposed to be identifying a click on the label "URL". The position of "URL" is Divider() on the right side and Divider() - StringWidth(Label()) on the left side. The top and bottom could be either Bounds().top and Bounds().bottom or with an inset. But hardcoding bottom to 15 will just break things when the user has a bigger font configured. Also nice will be overriding MouseMoved() and use the new BCursor(B_CURSOR_ID_FOLLOW_LINK) via SetViewCursor(). Since you will need the position of the URL label in both functions, it would be nice to code a new private method BRect _VisibleLabelBounds() const like this:

BRect
TTextControl::_VisibleLabelBounds() const
{
    BRect bounds(Bounds());
    // TODO: Could actually check current alignment of the label.
    // The code below works only for right aligned labels.
    bounds.right = Divider();
    bounds.left = bounds.right - StringWidth(Label());
   return bounds;
}

And then you can check the current mouse pos in MouseDown():

    if (_VisibleLabelBounds().Contains(mousePos))
        _LaunchURL(Text());
    else
        BTextControl::MouseDown(mousePos);

and in MouseMoved():

    if (_VisibleLabelBounds().Contains(mousePos)) {
        BCursor linkCursor(B_CURSOR_ID_FOLLOW_LINK);
        SetViewCursor(&linkCursor, true);
    } else
        SetViewCursor(B_SYSTEM_CURSOR_DEFAULT, true);

    BTextControl::MouseMoved(mousePos, ...);

(All from memory so may contain errors...)

As you can see, writing small functions like this can greatly improve the code quality and readability. It is totally clear what happens in MouseDown() now, even without any comments. While it wouldn't be clear at all in your version of the code, unless one reads the complete description of this ticket. :-)

[kitallis]

Here's an improved version that calls the gFields[] from PeopleApp.h and checks for specific labels with the current label in the TextControl fields (currently handling E-mail and URL). It also handles ​ftp:// now. It does not handle MouseMoved() as BTextControl cannot directly call it. It also handles ​ftp:// now. I'd like some reviews on this and possible improvements?

[stippi]

Thanks for the updated patch. Since I've already provided coding snippets, I want to point out some coding style things to improve:

• Private methods get the underscore prefix, as shown in my snippet.
• Variable names in parameters never have the f prefix, that's for member variables. Also "label" is wrong, since it's the text, not the label.
• The naming of LoadLabels() is not good. It sounds like the control loads label strings from somewhere and updates its label. How about "HandleLabelInvokation()" or "HandleLabelClicked()". It could also be a private method, with underscore prefix then.
• Why not put the check for mailto: into an else switch?
• I think my suggestion to add a TODO about _VisibleLabelBounds() only working for right-aligned labels was a good one. :-)
• I don't know what you mean about MouseMoved(). It should work just fine, just not over the text input, but over the label. It is good style to indicate this otherwise hidden feature by changing the mouse cursor shape.

[kitallis]

sorry about the multiple posts, some weird whitespace problems were creeping in.

[pulkomandy]

I did some tests with your patch.

• It does not apply cleanly, as there was some cleanup of the sourcecode
• You set the link cursor in mousemoved for all the fields, but only too of them are actually clickable
• For some reason the handling of URLs seems to work only once. Not sure what happens exactly.
• There were some style issues as well. Take care of whitespace (mixing tabs and space, and putting useless whitespace at end of lines), 2 blank lines between functions, and 80char limit.

[animux]

patch on top of the current master

[humdinger]

The patch applies to the current revision and seems to work. Others would have to comment on the code quality...
Now we're missing just one more thing: an indication that "URL" and "Email" is a hyperlink. Maybe by using a blue font colour like in AboutSystem?

[nielx]

Gentle reminder that the patch might need a small touch-up before it really can go in...

[mmadia]

To note, the patch applies cleanly. Also, when hovering the mouse over URL and E-Mail, the mouse cursor will change to B_CURSOR_ID_FOLLOW_LINK. Whether or not that is enough of an indicator, I'm not sure.

[richienyhus]

Only round brackets work on easy tasks page.

[pulkomandy]

Applied with minor changes in hrev50089.