Context menu for TextInput

[dancxjo]

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.

[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.

[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.

[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.

[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.

[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!

[dancxjo]

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

[dancxjo]

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

[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.

[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.

[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.

[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!

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

[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! :)

[leavengood]

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.

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

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

[leavengood]

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.