Opened 10 years ago

Closed 7 years ago

Last modified 7 years ago

#4592 closed enhancement (fixed)

Keyboard navigating alerts

Reported by: humdinger Owned by: humdinger
Priority: normal Milestone: R1
Component: User Interface Version: R1/alpha1
Keywords: Cc:
Blocked By: Blocking:
Has a Patch: yes Platform: All

Description

You can move through the buttons of an alert with tab/shift+tab. I'd find it useful to have the cursor keys work, too. And don't wrap the button-switching around, i.e. once you reached the left-most button, the left-cursor-key won't jump to the right-most button, but stays on the left-most.
That way if I know I want the left-most button, I hit cursor-left a few times and engage with return/space.

Plus, have ESC always cancel.

See also #2615 for using triggers.

Attachments (1)

0001-Close-alerts-with-ESCAPE-key.patch (177.1 KB ) - added by humdinger 7 years ago.
Patch to close BAlerts with ESC key.

Download all attachments as: .zip

Change History (12)

comment:1 by leavengood, 8 years ago

Owner: changed from stippi to leavengood
Status: newassigned

Taking ownership as I'm in shortcut coding mode.

comment:2 by axeld, 8 years ago

I'm not a friend of having the cursor keys do anything here; they are never used in this way anywhere, and I don't quite see why we should introduce that inconsistent behaviour.

+1 for the escape key, although that can't really be implemented in BAlert, as it's not always clear which option should be selected in this case (there is not always a cancel case in a BAlert).

comment:3 by leavengood, 7 years ago

Given Axel's response (which makes sense to me), is there really anything left in this to do? In fact if an API client wants escape to close the alert they can add B_CLOSE_ON_ESCAPE with SetFlags. In fact Humdinger if you want to add that to various alerts in included apps (where it makes sense) I think you have the skills at this point :)

comment:4 by humdinger, 7 years ago

Owner: changed from leavengood to humdinger

I'll look into it. Wouldn't SetShortcut(index, B_ESCAPE) (with the index being the cancel button) work better? I was wondering what return value a B_CLOSE_ON_ESCAPE flag would produce, which will be analyzed by following code...

in reply to:  4 comment:5 by leavengood, 7 years ago

Replying to humdinger:

I'll look into it. Wouldn't SetShortcut(index, B_ESCAPE) (with the index being the cancel button) work better?

Yeah that would probably be better, for those dialogs which have a Cancel (though IMO dialogs without a Cancel are bad UI design.)

I was wondering what return value a B_CLOSE_ON_ESCAPE flag would produce, which will be analyzed by following code...

Sorry I should have told you that, because I did investigate. Basically the return value is -1 if the window is send B_QUIT_REQUESTED (or is otherwise quit.) That is all B_CLOSE_ON_ESCAPE does within BWindow's keyboard handling.

comment:6 by humdinger, 7 years ago

I've attached a patch. If nobody points out what I might have done wrong, I'll commit it.

by humdinger, 7 years ago

Patch to close BAlerts with ESC key.

comment:7 by humdinger, 7 years ago

Has a Patch: set

comment:8 by leavengood, 7 years ago

Well take note that I said you can support closing on escape by ADDING B_CLOSE_ON_ESCAPE to the Flags. In the patch you set the flags to that alone, which probably removes the other flags which BAlert sets (B_NOT_CLOSABLE | B_NOT_RESIZABLE | B_ASYNCHRONOUS_CONTROLS). I haven't tested your patch so I can't be sure, but I strongly suspect that is the case. This is easy to fix though, probably even with a search and replace in your patch. Basically all instances of:

alert->SetFlags(B_CLOSE_ON_ESCAPE);

need to become:

alert->SetFlags(alert->Flags() | B_CLOSE_ON_ESCAPE);

That's how you add that flag without affecting the other ones. See:

http://www.haiku-os.org/legacy-docs/bebook/BWindow.html#BWindow_SetFlags

Though considering that BAlert already has the flag B_NOT_CLOSABLE, it could be argued that B_CLOSE_ON_ESCAPE sort of conflicts with that, at least in principle. It should still work technically, since the BWindow code which handles B_CLOSE_ON_ESCAPE doesn't look at B_NOT_CLOSABLE.

But taking a quick look at your patch, it seems that adding this will be fine for all those alerts.

comment:9 by humdinger, 7 years ago

Resolution: fixed
Status: assignedclosed

Thanks, Ryan, I missed that... I have applied an accordingly changed patch with hrev44473. Closing this ticket as fixed.

comment:10 by diver, 7 years ago

alert windows created with alert cli app still don't react on Esc. Too bad that cursor keys idea didn't get through, I find it very useful.

in reply to:  10 comment:11 by leavengood, 7 years ago

Replying to diver:

alert windows created with alert cli app still don't react on Esc.

That would be pretty trivial to add, though taking a quick look I don't know how well that program and clients of it would react to a -1 return value on Go().

Too bad that cursor keys idea didn't get through, I find it very useful.

Yeah, I would not mind it either, but it is inconsistent as Axel says.

Note: See TracTickets for help on using tickets.