Freeciv

Dean Brown wrote in #note-5:

The fix for #1718, which added menu items to change sound volume using "<" and ">" keys, caused those characters to also get blocked in the chat line.

So, there's currently a regression relative to 3.2.1. Makes this fix release critical (blocker) for 3.2.2.

Doesn't making the volume menu entries MGROUP_PLAYER cause e.g. them to be always disabled for global observers? In any case that is quite a hack, misusing the group definition.

I think the volume keys issue deserves ticket of its own, as it's different and separate change to the 'c' issue. (It's now a bit unfortunate that you deleted your earlier patches that did what this ticket is about)

You shouldn't unconditionally enable all menu items when chatline loses focus - they might should be disabled for other reasons.

Thinking more about this -

You shouldn't unconditionally enable all menu items when chatline loses focus - they might should be disabled for other reasons.

Good point, the code in il_lost_focus() should stay as is, just calling real_menus_update();

The current code in il_gained_focus() is also misusing the group definition. I thought when making the volume menu entries MGROUP_PLAYER that was a hack, but calling menus_disable_unit_commands() is also a hack. I have 2 proposals for doing the Right Thing in il_gained_focus() -

1 use code like this, modeled on menu_entry_group_set_sensitive()

  int i;

  for (i = 0; menu_entries[i].key != NULL; i++) {
    if ( Some test of menu_entries[i].accl ) {
      menu_entry_set_sensitive_info(map, &(menu_entries[i]), FALSE);
    }
  }

2 Redo the group definition system by changing the enum menu_entry_grouping so that the values are bit flags, e.g.

enum menu_entry_grouping { MGROUP_SAFE = 0B00000001, MGROUP_EDIT = 0B00000010, ...

  { "VOLUME_UP", N_("Volume Up"),
    "volume_up", "greater", MGROUP_SAFE | MGROUP_CHAR,
    NULL, FALSE },

I wonder if we could simply disable menus completely / disable all menu entries. "Entries with shortcut" are their functionality-wise random (odd) selection to have disabled, and user is not going to use shortcuts of those entries that don't have one, even if they would be enabled.
I just am not sure what it would do to accessing menus with a mouse when focus has been in chatline before that attempted menu access. Likely one couldn't click menus at all, so one would need to do extra click somewhere else for moving the focus.

I'm OK with disable menus completely / disable all menu entries. Could make an argument that the Quit item should be available.

If il_gained_focus() calls enable_menus(FALSE); then il_lost_focus() needs to call enable_menus(TRUE); and doesn't need to call real_menus_update(); - what happens then is the whole menu bar disappears/reappears, which is a bit disconcerting. But the question of accessing menus while chatline has focus is moot, can't access what's not there.

il_gained_focus() cannot call menu_entry_group_set_sensitive(G_ACTION_MAP(gui_app()), MGROUP_ALL, FALSE); directly because the enum is local to menu.c, need another routine in menu.c menus_disable_all_commands(). When I tried this it didn't work well - real_menus_update() failed to restore some items (notably "Quit" and "F1" - "F12"). Bugs here? Or real_menus_update() needs to do more. Would need more digging. You can still access the menus.

Changed my mind about disable menus completely / disable all menu entries. I use the "Merged" setting for client option "Messages and Chat reports location", so when I'm in the Chat tab I can't see the map view. When chat has focus, I would still want F1 to switch to the map view, so F1 menu item would have to be still enabled.

Patch does not apply to S3_3. I guess it needs patch of its own (the existing patches apply to main & S3_2, and are in my work trees of those branches now)

Dean Brown wrote in #note-15:

real_menus_update() failed to restore some items (notably "Quit" and "F1" - "F12"). Bugs here?

real_menus_update() never makes sensitive those menu items that cannot go insensitive in the first place. So when you make it possible for new items to go insensitive, you need to also add the code to make them sensitive again.

Marko Lindqvist wrote in #note-24:

Noticed on main: A)utoworker does not work with this patch. The entry is completely gone from the menus.

Same bug is also in S3_3.