| | [ Return to Bugs & Features | Roadmap 1.3 | SVN ⇄ GIT ]
STR #2243
| Application: | FLTK Library |
|---|
| Status: | 1 - Closed w/Resolution |
|---|
| Priority: | 4 - High, e.g. key functionality not working |
|---|
| Scope: | 2 - Specific to an operating system |
|---|
| Subsystem: | Core Library |
|---|
| Summary: | Fl_Menu_Bar on windows: single letter shortcut keys not working |
|---|
| Version: | 1.3-current |
|---|
| Created By: | greg.ercolano |
|---|
| Assigned To: | AlbrechtS |
|---|
| Fix Version: | 1.3.0 (SVN: v7816) |
|---|
| Update Notification: | |
|---|
Trouble Report Files:
Trouble Report Comments:
| |
| #1 | greg.ercolano
23:06 Aug 28, 2009 |
| For instance, one should be able to quit the app with
either of these sequences from the keyboard:
1) (ALT-f), (ALT-x)
2) (ALT-f), (x)
#1 works, but #2 does not. Replication:
1) Run test\editor.exe
2) Hit ALT-F to open the "File" menu
3) Hit just 'x' (no ALT key) to try to quit, but it does not
Works fine on Linux. | |
| |
| #2 | andek
12:41 Feb 09, 2010 |
| I would like to give this high priority, since it stops us from using the latest 1.3 in the next release of our software. | |
| |
| #3 | greg.ercolano
16:44 Feb 09, 2010 |
| I should add, I think I was describing the test/menubar application.
However currently (svn 7052) it seems to use 'q' to quit instead of 'x'. | |
| |
| #4 | AlbrechtS
05:11 Feb 10, 2010 |
| Raised priority as requested. I consider this as a serious bug because it is a regression from previous versions and works differently on other platforms. | |
| |
| #5 | matt
01:38 Apr 05, 2010 |
| Any takers with a Windows machine? | |
| |
| Hi.
I don't know if this can help you, but also 1.1.10 seems to have this problem. Only 1.1.9 works properly.
That is: compiling menubar.cxx (or editor.cxx) from tests with 1.1.9 installed, everything is ok.
If 1.1.10 or 1.3 are installed, the problem comes out (in Windows XP-sp3, at least).
Bye | |
| |
| #7 | greg.ercolano
08:50 Jun 29, 2010 |
| > 1.1.10 seems to have this problem. Only 1.1.9 works properly.
That should help a lot.
I'm xdiff'ing the source for 1.1.9 and 1.1.10 now
to see if I can help spot the problem. | |
| |
| #8 | greg.ercolano
13:15 Jun 29, 2010 |
| I'm suspecting the problem /might/ be the following new code added in to fl_shortcut.cxx in 1.1.10 and 1.3.x:
----
#ifdef WIN32
// on MSWindows, users expect shortcuts to work only when the Alt modifier is pressed
if (Fl::event_state(FL_ALT)==0) return 0;
#endif
----
The above apparently is to fix STR #2199:
http://www.fltk.org/str.php?L2199
I tried taking the latest FLTK 1.3.x to test if removing that code helps, but unfortunately I cant build FLTK 1.3.x on windows at the moment (due to STR #2393).
Anyone wanting to fix this might try commenting out the above code from fl_shortcut.cxx to see if that solves it. | |
| |
| #9 | greg.ercolano
17:30 Jun 29, 2010 |
| OK, just got FLTK to build on windows.
Indeed, those lines that were added to fix STR #2199 seems to be the cause of the problem; commenting them out makes the keys work again.
I guess we need to either back that out, or make appropriate changes to that code.
Matt, I think this was your fix.. maybe the fix for STR #2199 can be re-approached with the current STR in mind?
Possibly the problem causing STR#2199 is elsewhere, as we know there are some issues floating around in 1.3.x with respect to state keys across the platforms. (eg. STR#2366) | |
| |
| #10 | andek
16:36 Aug 14, 2010 |
| What is the status of this issue? I think it's very serious, it stops us from updating to the latest FLTK 1.3 revision, since our users won't accept suddenly having to press Alt to reach menu items. | |
| |
| #11 | greg.ercolano
17:41 Aug 16, 2010 |
| Awaiting response from Matt.. | |
| |
| #12 | andek
13:32 Aug 18, 2010 |
| Is Matt still active here? | |
| |
| #13 | andek
11:50 Sep 12, 2010 |
| Since Matt doesn't seem to be covering this issue, does anyone else have an idea of how to solve it? It would be much appreciated. | |
| |
| #14 | greg.ercolano
19:18 Sep 12, 2010 |
| I believe commenting out this line is the recommended fix at the moment. eg. edit fl_shortcut.cxx, find this line:
if (Fl::event_state(FL_ALT)==0) return 0;
..within the #ifdef/#endif markers (see above) and comment it out, and rebuild.
This fixes the ALT key problem, but may cause STR#2199 again,
but that STR is not really a "problem", it just closes some optional
behavior the poster didn't like, IIRC. | |
| |
| #15 | andek
10:31 Sep 13, 2010 |
| I totally agree, #2199 is far less serious than #2243. I suggest that these lines of code can be commented out as soon as possible.
/Andreas | |
| |
| #16 | AlbrechtS
15:17 Sep 15, 2010 |
| After some discussion in fltk.development, there is a proposal for a patch written by Andreas Ekstrand (andek), based on some ideas discussed today[1]. Andreas wrote in fltk.development[2]:
"In Fl_Menu_Item.H, I changed the find_shortcut declaration to:
const Fl_Menu_Item* find_shortcut(int *ip=0, const bool &require_alt = false) const;
In Fl_Widget.H, I changed the test_shortcut declaration to:
static int test_shortcut(const char*, const bool &require_alt = false);
In FL_Menu.cxx, I propagate the require_alt parameter in Fl_Menu_Item::find_shortcut to Fl_Widget::test_shortcut.
In Fl_Menu_Bar.cxx, I send (0, true) as parameters to find_shortcut.
In fl_shortcut.cxx, I only check the FL_ALT state if require_alt is true (still only for WIN32)."
For more info please check the links [1] and [2] below.
I tried to translate the description into a diff/patch file and uploaded it to this STR for reference [3]. More comments to follow...
[1] http://www.fltk.org/newsgroups.php?gfltk.development+Q%22STR+%232243%22
[2] http://www.fltk.org/newsgroups.php?gfltk.development+v:10668
[3] http://www.fltk.org/strfiles/2243/str_2243_v1.patch | |
| |
| #17 | AlbrechtS
15:30 Sep 15, 2010 |
| Andreas, thanks for the description and testing. Is the uploaded patch what you described?
I did not test or verify the patch yet, but from reading the description, this might be what we need...
Some technical comments:
(1) in FLTK 1 we usually don't use bool, but use int instead. I don't know the (historical ?) reasons, but maybe we should adjust this.
(2) why "const bool &require_alt", i.e. why should we use a reference argument here? This could also be by value... (I see the advantage of the const property, but I don't know.) Anybody?
As I wrote before, I won't have the time to test it until the next weekend. All testers and opinions welcome... | |
| |
| #18 | AlbrechtS
15:47 Sep 15, 2010 |
| Okay, now a few quick test results, using test/menubar:
First: there are multiple menu parts with identical shortcuts and/or only different in case, e.g. &File/&Font, &Checkbox/&choice, E&mpty/&menubutton...
Alt+F opens File menu (okay: Font menu can't be used by a shortcut).
Submenus &Close and &Quit work with and w/o Alt: okay.
Alt+R triggers Edit/Redo (instead of &Radio?) - why?
Alt+M opens the menubutton menu (and not the E&mpty menu) - why?
Alt+C triggers &Edit/Copy (Alt+C) (instead of &Checkbox?) - why?
Parts of the odd (unexpected) behavior might be in older FLTK versions as well, I don't know...
Anyway, more tests needed... | |
| |
| #19 | andek
13:16 Sep 16, 2010 |
| Sorry for my complicated report, I'm not used to patch files yet, but I will learn, I promise. :-)
I also noticed that the menubar example contains a lot of duplicate shortcuts. Some ideas of the things you found:
> Alt+R triggers Edit/Redo (instead of &Radio?) - why?
The Redo menu item actually has an explicit Alt+R shortcut which probably overrides, an obvious mistake in the menubar example design I think.
> Alt+M opens the menubutton menu (and not the E&mpty menu) - why?
The menubutton which also has 'm' as shortcut overrides, but maybe we should forbid the use of Alt here instead? Like the opposite of a top level menu bar? I'm not sure what's expected here.
> Alt+C triggers &Edit/Copy (Alt+C) (instead of &Checkbox?) - why?
Same as with Edit/Redo above, the Alt+C explicit shortcut overrides, another obvious mistake in the menubar design I think.
Seems to me that menubutton vs. E&mpty is the only uncertainty left? Perhaps giving Fl_Menu_Bars higher priority somehow could be a general solution? Or shouldn't Alt be allowed for menubutton?
/Andreas | |
| |
| #20 | andek
13:24 Sep 16, 2010 |
| ...and yes, the patch is exactly what I described, thanks for converting my report into something useful.
Regarding the bool/int issue, I have no objections - other than that bool is of course more clear and perhaps more efficient, but it's probably more important to be consistent with the rest of the code.
Regarding passing require_alt as a reference, that's just a habit I have when coding C++. I realize it's more important with more expensive objects performance-wise but I have just adopted that habit as a C++ standard way of coding. It's of no real importance to me.
/Andreas | |
| |
| #21 | AlbrechtS
03:59 Sep 23, 2010 |
| Greg, do you think that the provided patch is okay for this STR and for STR 2199 as well? Did anybody else look at this? I'd like to get this out of the way...
If it's okay, we should also think about making this platform-independent and not only for WIN32 - maybe...
Are there any further comments? | |
| |
| #22 | andek
13:48 Oct 10, 2010 |
| No objections to my proposed changes? Have you had the time to look at this, Greg? | |
| |
| #23 | greg.ercolano
16:43 Oct 10, 2010 |
| Hard for me to weigh in further on this, as I'm not at all familiar with WIN32 events in FLTK.
Someone very familiar with keyboard events in Windows w/fltk should do the final polish on the solution.
I'm not the right person to fix, just reported the problem and did some initial sleuthing to try to find out where in the history the problem started.
I don't know the right solution to solve both STR's (2199 and 2243), but if the patch seems the right way, or even if not the best way but solves both STR's, I'm all for it. | |
| |
| #24 | greg.ercolano
18:35 Oct 10, 2010 |
| I tried applying the attached V1 patch to SVN 7711 and rebuilt FLTK on linux and win32. Some comments:
1) Menubar's *menu* shortcuts seem to work more consistently now;
(Alt-F - Alt-Q) will quit the application, and so will
(Alt-F - q)
..which is definitely an improvement; great!
Seems to solve *this* STR.
2) Related STR#2199 seems broken again (or maybe always has been?)
If I understand that STR correctly, under windows ALT is needed
to trigger button shortcuts.
However, in both 'menubar' and 'button' test programs,
both of those app's buttons can still be triggered with
regular letter keys under Windows:
* 'menubar.exe': just hitting 'm' triggers the 'menubutton'
if it has focus.
* in 'button.exe', hitting any of these triggers the button
shortcuts:
'b', SHIFT-b, ALT-b,
'n', SHIFT-n, ALT-n,
'x', SHIFT-x, ALT-x
I think STR#2199 was supposed to limit button shortcuts
to be triggered *only* if ALT is included in the sequence.
3) Just something I noticed; the #ifdef/#endif in the test_shortcut()
function of fl_shortcut.cxx are indented instead of flush-left.
Not sure why it builds without errors, but I think '#' macro
definitions must be flush left to work correctly.
Regarding #2, should we make sure any fix we come up with here
solves both STR's if possible..? | |
| |
| #25 | andek
08:13 Oct 11, 2010 |
| Hi,
Regarding #2, I think buttons are supposed to be triggered with regular letter keys, this is consistent with Windows behavior in general. My impression of the problem in STR #2199 was that buttons and menus could be triggered with Alt and Shift also, whereas the reporter thought it should be _only_ with regular keys. However, I don't recognize that as a problem, at least Visual Studio allows Alt+N, Shift+N or just plain N as shortcut to the No button when asking about saving a file that is closing, just as an example. So if it was only up to me, I think #2199 should be ignored and this patch makes everything okay.
Regarding #3, Visual Studio doesn't seem to care about indentation of #if/#endif, although auto-formatting the code will move them to the left. But if you commit this, just "flush-left" them I guess.
/Andreas | |
| |
| #26 | greg.ercolano
08:30 Oct 12, 2010 |
| andek: Re #2: No, STR #2199 only wanted the Alt modifier to trigger the button shortcuts, not Shift (or regular keys).
There are many good reasons for not wanting that, since it means a user doing regular typing might accidentally trigger button shortcuts. (eg. if the user thought they were typing in an input field, but the input field didn't have focus at the time..)
Example: An interface to control industrial machinery that has two buttons, &Start and &All Stop., and an input field. A user wants to type the word "Sally" into the input field, but forgets to give the input field focus; The 'S' and 'a' in 'Sally' would trigger both the "Start" and "All Stop" buttons accidentally, starting and stopping the machinery unexpectedly.
As STR#2199 reads (OP):
Shift+letter activates button/label shortcuts, same as alt+letter.
[..] I have never before used shift as activation key for shortcuts
in any program.
Also: there is also a follow up in that STR comparing linux and windows behavior of button shortcuts, using Firefox's own behavior as an example, showing letter codes don't work for buttons in windows, but do in linux:
http://fltk.org/newsgroups.php?gfltk.general+v:28192 | |
| |
| #27 | greg.ercolano
09:06 Oct 12, 2010 |
| andek: cont'd
However, Albrecht (one of our developers, not the OP) seemed to follow up on STR#2199 that he's seen native windows apps where regular letters trigger buttons.
Matt, one of our other developers, applied a patch that forced Alt to trigger shortcuts only on Windows, which closed that STR, and is the modification that caused the current issue, STR #2243.
I guess we should decouple these two STRs, and apply Albrecht's patch, and treat the other issue in #2199 as a separate issue for the devs to possibly discuss further on fltk.development to see if it needs further attention. | |
| |
| #28 | AlbrechtS
10:00 Oct 12, 2010 |
| For the record: "Albrecht's patch" is in fact Andreas's (andek's) patch - only _posted_ by me.
I still didn't completely analyze the patch (look for regressions or something else?), but I believe that this patch or something very similar should be implemented.
I know that we had an app. that relied on shortcuts w/o ALT (or SHIFT) being pressed. Users are often asking for such features. Although I can see the danger Greg described about accidentally triggering callbacks, I think that triggering button callbacks by pressing keys w/o any modifiers may be useful. Of course, the app should be designed to do it the right way.
OTOH, I do also agree that the menubar shortcuts should only be fired by also pressing ALT (as it is usually done in most/all software I know). This applies only to the first level of a menu, though - further actions should be usable w/o holding ALT.
This all seems to be what the current patch does. It may, however, revert (parts of) a change done in STR #2199 which I personally consider ... suboptimal ;-). Even Matt was not sure about it: "I changed the behavior for Windows. We will see if there are any reactions by the users." Unfortunately it took very long for reactions, so that we released FLTK 1.1.10 with this state.
That said, I'd also vote for removing the WIN32 conditional code to make it the same on all platforms - since there are examples for similar behavior on different platforms, as Greg mentioned above. | |
| |
| #29 | andek
15:22 Oct 16, 2010 |
| Okay, then - seems like we are on the right track here.
Regarding being able to activate buttons with their shortcut letters only, I think a common example of where you want this is a question dialog when closing a document, asking if the user wants to save the document first. I usually press 'n' in such a dialog. In Office 2003 e.g., this is possible (just tried it as an example). However, I can see the danger described for this behavior as well, but I guess the application should be designed to avoid such situations, e.g. by not setting shortcuts for important buttons in dialogs with text boxes.
Hoping for a commit of my patch or something very similar soon... | |
| |
| #30 | andek
13:53 Oct 25, 2010 |
| Any plans for submitting something soon? | |
| |
| #31 | greg.ercolano
16:28 Oct 25, 2010 |
| If I understand it correctly, Albrecht's current recommendation is:
1) Apply the patch
2) Remove the #ifdef WIN32 in fl_shortcut.cxx
If so, I'll be happy to do those two things and check it in. | |
| |
| #32 | andek
13:40 Nov 01, 2010 |
| Albrecht, do you agree?
I hate to be a pain in the butt here, and please tell me if I'm out of line with my nagging, but I'm really anxious to get this fixed in the 1.3 trunk so I can make use of all the other good stuff happening there and still relying on a committed revision and not only my local modifications. | |
| |
| #33 | AlbrechtS
01:36 Nov 02, 2010 |
| Andreas and Greg, sorry, I've been (almost) offline for a while. I believe that we agreed to fix this as discussed and as Greg summarized above.
I'd like to modify the patch, though, to adhere to the FLTK coding standards as discussed above (int instead of bool, no reference argument for int). I will do this shortly, if nobody else jumps in... | |
| |
| #34 | AlbrechtS
08:19 Nov 10, 2010 |
| I'm preparing the modified patch including documentation updates. The current patch uses bool, but I'm waiting for more comments in fltk.development regarding usage of "bool". I'm ready to commit the patch once this is decided... | |
| |
| #35 | AlbrechtS
01:23 Nov 11, 2010 |
| Fixed in Subversion repository.
I committed the changes in svn r7816. This also updates the docs, thus it is a much bigger update than the simple patch proposed, but it is essentially the same.
(1) The Alt key is required only for the first level of menus (menubars). This reverts the unconditional change (for Windows only) in STR #2199 (r6802).
(2) This change is platform-independent (no special case for Windows).
(3) I removed the reference for the bool argument, but left it "bool".
(4) This all means that single letter shortcuts work for buttons w/o Alt modifier, if no other input widget has the focus.
Please test and verify the patch. This STR will be closed in a few days, if no problems are reported. | |
| |
| #36 | andek
14:06 Nov 11, 2010 |
| Great! It works fine. I'm happy. :-) | |
| |
| #37 | matt
07:19 Nov 14, 2010 |
| Fixed in Subversion repository. | |
[ Return to Bugs & Features ]
|
| |
