Opened 8 years ago

Closed 8 years ago

#7002 closed enhancement (fixed)

Context menu for TextInput

Reported by: dancxjo Owned by: leavengood
Priority: normal Milestone: R1
Component: Kits/Interface Kit Version: R1/Development
Keywords: context menu text edit textinput Cc:
Blocked By: Blocking:
Has a Patch: yes Platform: All

Description

I've always been annoyed by the lack of a context menu when editing text in Haiku and BeOS. I noticed WebPositive and other apps implemented this themselves, which seems like a bad idea. There are some glitches with the keyboard shortcuts, but maybe it can be included anyway and others can fix it.

Attachments (1)

ClipboardMenu.diff (4.6 KB) - added by dancxjo 8 years ago.

Download all attachments as: .zip

Change History (20)

comment:1 Changed 8 years ago by dancxjo

Has a Patch: set

comment:2 Changed 8 years ago by diver

Component: - GeneralKits/Interface Kit
Owner: changed from nobody to axeld

comment:3 Changed 8 years ago by stippi

Patch looks mostly Ok and we definitely want to have the feature. However, you should read the Coding Style guide lines again. I don't have the time to repeat them here and point out every style violation, there are quite a few in this patch. What you do with regards to using the locale backend seems pretty weird to me. Also, you need to define a message constant somewhere, and it needs to be upper case, as all system message codes are. Since there are two methods called in the BTextInput constructor now, there should be a private _Init() method refactored from these.

comment:4 Changed 8 years ago by dancxjo

I agree that the locale backend stuff is bizarre, but I pretty much just copied it from ColorControl.cpp. I've updated the patch to use click to open. I will fix the style violations now.

comment:5 Changed 8 years ago by dancxjo

Okay...I think I've got most of the coding violations. Sorry about that. I'm not sure if I've declared the kClearInput constant correctly though. Please let me know about other violations I may have left.

I took out the CMD+Z shortcut. BeBook says it should work, but in Haiku it doesn't.

comment:6 Changed 8 years ago by dancxjo

I noticed some whitespace style violations. Ooops. I also added some logic to prevent people from editing text when they aren't supposed to.

comment:7 Changed 8 years ago by axeld

One of the coding style violations that you manage to disobey almost consequently is the "two blank lines between sections" rule. You even changed existing code to remove it at some places, that is usually a bad idea.

Also, the declaration of the menu items does not conform to the style guide; you should only declare one variable per type.

kClearInput should probably be renamed to kMsgClearInput, and its constant should be a system code, like those in defined in the public headers, IIRC they are prefixed with '_'.

There are more style violations in there, please have another look at the coding style guide, and thanks for the code - the rest looks pretty much okay!

comment:8 Changed 8 years ago by dancxjo

As to kMsgClearInput, shouldn't it be symmetrical with B_PASTE ('PSTE'), B_UNDO ('UNDO') et al.?

comment:9 Changed 8 years ago by dancxjo

This time, I used the handy-dandy vim style checker to make sure I was doing right.

comment:10 Changed 8 years ago by bonefish

BTW, I can wholeheartedly recommend the Menu layout builder from <LayoutBuilders.h> (e.g. cf. the Terminal menu code). This should make things more compact and pleasant to read, and if you determine first which items need to be enabled/disabled, it should even be possible to get rid of all the BMenuItem* variables.

The style violations I spotted at a quick glance:

  • is_redo -> isRedo
  • The else block has two lines and should therefore be enclosed in braces.

comment:11 in reply to:  10 ; Changed 8 years ago by dancxjo

Really awesome suggestion on the layout builder! I'm not sure if it was correct of me to indent the .SetEnabled's in the layout builder, though. Your thoughts? I want to ask PulkoMandy if there is a cleaner way to handle all those translated strings. It would be nice to be able to just use an inline B_TRANSLATE.

comment:12 in reply to:  11 ; Changed 8 years ago by bonefish

Replying to dancxjo:

Really awesome suggestion on the layout builder! I'm not sure if it was correct of me to indent the .SetEnabled's in the layout builder, though. Your thoughts?

Strictly according to the coding guidelines the indentation would be incorrect. However, for layout building code an indentation outlining the structure of the layout significantly increases the readability and is therefore preferred. Indenting the .SetEnabled() calls is fine, as they affect the previous item, not the menu.

Regarding the style guide:

  • Two blank lines should be used between the #include and the using blocks, between the using and the B_TRANSLATE_CONTEXT blocks, and between the latter and the namespace ....
  • The { for a function/method definition goes to a separate line.
  • Uncommon abbreviations for identifier names should be avoided, i.e. e.g. undoStr should be renamed to undoString/undoName/undoLabel.
  • BPopUpMenu *menu;

Moreover:

  • For aesthetical reasons I would move the creation of the BPopUpMenu object to the definition of the variable.
  • I would cache the IsEditable() result in a variable to avoid multiple calls.
  • B_UNDO is erroneously used for the redo menu item.

I want to ask PulkoMandy if there is a cleaner way to handle all those translated strings. It would be nice to be able to just use an inline B_TRANSLATE.

The problem is that this is libbe and liblocale depends on libbe. Directly using liblocale functionality in libbe would make dependencies cyclic. Hence the unusual way to get the translated string. Things could be simplified by introducing a macro specially for libbe code (e.g. B_TRANSLATE_LIBBE), which would do exactly what has now to be programmed by hand. I believe it needs to be supported explicitly by collectcatkeys, though. Another option would be to merge liblocale into libbe. To avoid loading the largish ICU libraries when not needed, that would require a dynamically loaded ICU bridge, though.

comment:13 in reply to:  12 Changed 8 years ago by dancxjo

  • Uncommon abbreviations for identifier names should be avoided, i.e. e.g. undoStr should be renamed to undoString/undoName/undoLabel.

Removed altogether; but is the macro okay? The name is perhaps too generic and it assumes that the Locale Backend isn't null. Is this a bad idea???

  • B_UNDO is erroneously used for the redo menu item.

There is no such thing as a B_REDO, AFAIK. Redo is just undo when the last action was an undo. I'm not very pleased with it, but I didn't want to hack the undo buffer.

Thank you, bonefish, for the advice so far!

comment:14 Changed 8 years ago by stippi

The patch looks pretty clean now, however, there are some style violations with regards to variable declaration:

Wrong:

    if (buttons == B_SECONDARY_MOUSE_BUTTON) { 
        undo_state state; 
        bool isRedo, isUndo; 
        state = UndoState(&isRedo); 
        isUndo = state != B_UNDO_UNAVAILABLE && !isRedo; 

        int32 start, finish; 
        GetSelection(&start, &finish); 

Correct:

    if (buttons == B_SECONDARY_MOUSE_BUTTON) { 
        bool isRedo; 
        undo_state state = UndoState(&isRedo); 
        bool isUndo = state != B_UNDO_UNAVAILABLE && !isRedo; 

        int32 start;
        int32 finish; 
        GetSelection(&start, &finish); 

Thanks a lot for your work, hope you don't mind the picky reviews... :-)

Changed 8 years ago by dancxjo

Attachment: ClipboardMenu.diff added

comment:15 in reply to:  14 Changed 8 years ago by dancxjo

Thanks a lot for your work, hope you don't mind the picky reviews... :-)

I'm just sorry it's taken me this many tries to work out the style violations! :)

comment:16 Changed 8 years ago by leavengood

Owner: changed from axeld to leavengood
Status: newassigned

This is funny and somewhat annoying as I literally just wrote very similar code in WebPositive to improve the URL input's right click menu. Though I definitely agree it belongs in BTextInput, so I'll take over this issue.

I do wonder if we should just create a B_CLEAR message constant in AppDefs, since I was also surprised there was no such thing when changing the WebPositive code. There isn't anything similarly named and 'CLER' is even available.

comment:17 Changed 8 years ago by leavengood

I think the patch is fine but I wonder if this right click menu handling wouldn't be better placed in BTextInput's parent class BTextView. That way both classes (and other BTextView derivative classes) can benefit from this default menu.

Then on that line it might be nice if this menu could be exposed from BTextView so that child classes could add on to it. But that can be done later :)

comment:18 Changed 8 years ago by axeld

Sure, that would make sense - and I guess we would receive bug reports if some old application suddenly opens two context menus instead of one (which would require it to still call BTextView::MouseDown() which it really shouldn't do though in this case).

comment:19 Changed 8 years ago by leavengood

Resolution: fixed
Status: assignedclosed

Fixed in hrev40305 by adding this context menu to BTextView, as discussed. In the testing I did nothing seemed to act strangely. Thanks for the patch I was able to use most of it directly in BTextView.

Note: See TracTickets for help on using tickets.