Opened 10 years ago

Closed 4 years ago

#3825 closed enhancement (fixed)

URL linkification in People (easy)

Reported by: humdinger Owned by: waddlesplash
Priority: low Milestone: R1
Component: Applications/People Version: R1/Development
Keywords: gsoc2010, people, gsoc2012 Cc: alexander@…
Blocked By: Blocking:
Has a Patch: yes Platform: All

Description

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.

Attachments (4)

PeopleUrlFix.patch (1.9 KB ) - added by kitallis 9 years ago.
This sort of fixes up the verify feature of the URL. Clicking on the "URL" label text will launch the URL inside the field
People[#3825]2.patch (2.9 KB ) - added by kitallis 9 years ago.
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?
People[#3825].patch (3.8 KB ) - added by kitallis 9 years ago.
sorry about the multiple posts, some weird whitespace problems were creeping in.
URL-linkification-in-People.patch (5.2 KB ) - added by animux 7 years ago.
patch on top of the current master

Download all attachments as: .zip

Change History (23)

comment:1 by axeld, 10 years ago

Owner: changed from axeld to nobody
Version: R1/pre-alpha1

by kitallis, 9 years ago

Attachment: PeopleUrlFix.patch added

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

comment:2 by humdinger, 9 years ago

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

in reply to:  2 comment:3 by kitallis, 9 years ago

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

comment:4 by humdinger, 9 years ago

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

comment:5 by stippi, 9 years ago

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.

comment:6 by stippi, 9 years ago

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. :-)

comment:7 by kitallis, 9 years ago

Keywords: gsoc2010 people added

by kitallis, 9 years ago

Attachment: People[#3825]2.patch added

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?

comment:8 by stippi, 9 years ago

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.

by kitallis, 9 years ago

Attachment: People[#3825].patch added

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

comment:9 by mmadia, 9 years ago

Has a Patch: set

comment:10 by pulkomandy, 9 years ago

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.

comment:11 by anevilyak, 8 years ago

Has a Patch: unset

comment:12 by animux, 7 years ago

Cc: alexander@… added
Keywords: gsoc2012 added

by animux, 7 years ago

patch on top of the current master

comment:13 by animux, 7 years ago

Has a Patch: set

comment:14 by humdinger, 7 years ago

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?

comment:15 by nielx, 7 years ago

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

comment:16 by mmadia, 7 years ago

Owner: changed from nobody to stpere
Status: newassigned

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.

comment:17 by richienyhus, 6 years ago

Summary: URL linkification in People [easy]URL linkification in People (easy)

Only round brackets work on easy tasks page.

comment:18 by waddlesplash, 5 years ago

Owner: changed from stpere to waddlesplash
Version: R1/Development

comment:19 by pulkomandy, 4 years ago

Resolution: fixed
Status: assignedclosed

Applied with minor changes in hrev50089.

Note: See TracTickets for help on using tickets.