Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Separator in menu gets focus #675

Closed
dumblob opened this issue Jul 10, 2022 · 11 comments · Fixed by #690
Closed

Separator in menu gets focus #675

dumblob opened this issue Jul 10, 2022 · 11 comments · Fixed by #690
Assignees
Labels
type:bug Something's broken!
Projects
Milestone

Comments

@dumblob
Copy link

dumblob commented Jul 10, 2022

Upfront Information

  • Fvwm3 version (run: fvwm3 --version)
    fvwm3 1.0.5 (1.0.4-128-gc7b41a03) with support for: ReadLine, XPM, PNG, SVG, Shape, XShm, SM, Bidi text, XRandR, XRender, XCursor, XFT, NLS

  • Linux distribution or BSD name/version
    Arch Linux

  • Platform (run: uname -sp)
    Linux 5.15.53-1-lts #1 SMP x86_64 GNU/Linux

Expected Behaviour

Using arrow keys on keyboard to move through a menu should not focus separators/delimiters (+ "" Nop items). In FVWM2 separators did not get focus, but in recent git FVWM3 they do get focus.

Actual Behaviour

Separators get focus as if they were regular menu items. But they should not get any focus.

Enabling logging

N/A

Steps to Reproduce

Create menu:

DestroyMenu	FvwmRootMenu
AddToMenu   FvwmRootMenu
+ "$[gt.Console]%terminal%"    Exec exec xterm
+ "" Nop
+ "$[gt.Console2]%terminal%"    Exec exec xterm
  • Does the problem also happen with Fvwm2?

No.

Does Fvwm3 crash?

No

Extra Information

Some real menus look then like this (the top separator is focused by arrow selection and the bottom one is not focused):
20220710_213313+0200-1680x1050+0+0-imlib2_grab

(note the color style is chosen deliberately so that one can recognize which colors are being used for what part of the frame(s))

@dumblob dumblob added the type:bug Something's broken! label Jul 10, 2022
@ThomasAdam ThomasAdam added skip:changelog Issue/PR should skip CHANGELOG and removed type:bug Something's broken! labels Aug 26, 2022
@ThomasAdam ThomasAdam added this to To do in FVWM3 via automation Aug 26, 2022
@ThomasAdam ThomasAdam added this to the 1.0.5 milestone Aug 26, 2022
@ThomasAdam
Copy link
Member

Not a fvwm problem. I'm going to guess it's a graphics driver problem.

FVWM3 automation moved this from To do to Done Aug 26, 2022
@dumblob
Copy link
Author

dumblob commented Sep 2, 2022

Oh, this is a misunderstanding. With "focus" I am exclusively referring to keyboard focus, not any visual focus. In the following menu:

DestroyMenu FvwmRootMenu
AddToMenu FvwmRootMenu
+ "$[gt.Console]%terminal%"  Exec exec xterm
+ "" Nop
+ "$[gt.Console2]%terminal%"  Exec exec xterm

If I focus Console by keyboard arrows and then press the "down arrow" key on the keyboard it will not focus Console2 item in fvwm3 but rather the tiny "Nop" separator line. In fvwm2 doing the same does focus Console2.

This has really nothing to do with any graphics driver.

@somiaj
Copy link
Collaborator

somiaj commented Sep 2, 2022

I can confirm this as well, when using the keyboard to move through window items in a menu, it will hit the Nop separators, and require a second key press to move to the item below the separator. This is a "recent" change, as it worked as expected in 1.0.4, but a rebuild of current master the bug shows up. So something has changed since 1.0.4.

@somiaj somiaj reopened this Sep 2, 2022
FVWM3 automation moved this from Done to To do Sep 2, 2022
@ThomasAdam
Copy link
Member

Yes indeed. Thanks for the tip about using the keyboard.

After a little dance with git-bisect, this breakage was introduced in 91ef704

I am not too sure why this only happens with the menu. A proposed change might be the following:

diff --git a/fvwm/menuitem.c b/fvwm/menuitem.c
index beeea336e..34f186d9a 100644
--- a/fvwm/menuitem.c
+++ b/fvwm/menuitem.c
@@ -461,7 +461,8 @@ void menuitem_paint(
        }
        else
        {
-               MI_IS_SELECTABLE(mi) = True;
+               if (!MI_IS_SEPARATOR(mi))
+                       MI_IS_SELECTABLE(mi) = True;
                gcs = ST_MENU_INACTIVE_GCS(ms);
                off_gcs = ST_MENU_INACTIVE_GCS(ms);
        }

@topcat001 -- can you check if the above doesn't break the intent of your original change?

@somiaj / @dumblob -- if you could test the above patch, regardless of anything else, I'd appreciate it.

@ThomasAdam ThomasAdam added type:bug Something's broken! and removed skip:changelog Issue/PR should skip CHANGELOG labels Sep 2, 2022
@topcat001
Copy link
Contributor

topcat001 commented Sep 2, 2022

Aha! I missed this case because I don't usually use the keyboard to navigate menus. Thanks! I'll test this and report back. Fairly sure this fixes it.

@topcat001
Copy link
Contributor

@ThomasAdam works for me. Thanks!

@somiaj
Copy link
Collaborator

somiaj commented Sep 3, 2022

This fixes the issue here. Side comment, I do notice if you mouse over the separator exactly, both the item above and below lose focus, but the separator isn't gaining focus and the keyboard movement looks like what is expected (fixing the actual issue).

@topcat001
Copy link
Contributor

topcat001 commented Sep 3, 2022 via email

@somiaj
Copy link
Collaborator

somiaj commented Sep 3, 2022

I never looked at what the original commit was suppose to fix/change, just stating that this fixes this issue with the keyboard movement, and what I noticed with mousing over a separator.

@topcat001
Copy link
Contributor

topcat001 commented Sep 3, 2022 via email

@dumblob
Copy link
Author

dumblob commented Sep 3, 2022

Seems to work for me - I use keyboard exclusively to navigate menus. I did not test (read compare to fvwm2) mouse behavior as that is not priority for me 😉 - but if you insisted, I will test the mouse behavior too.

ThomasAdam added a commit that referenced this issue Sep 4, 2022
When setting whether a painted menu item is selectable or not, ensure
this does not happen for separators, as they can gain focus without this
check, but doing so is not helpful.

Fixes #675
@ThomasAdam ThomasAdam self-assigned this Sep 4, 2022
ThomasAdam added a commit that referenced this issue Sep 4, 2022
When setting whether a painted menu item is selectable or not, ensure
this does not happen for separators, as they can gain focus without this
check, but doing so is not helpful.

Fixes #675
FVWM3 automation moved this from To do to Done Sep 4, 2022
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this issue Oct 19, 2022
(One of the more significant changes is that locking issues with libX11
1.8.1 are fixed.)

## [1.0.5](https://github.com/fvwmorg/fvwm3/tree/1.0.5) (2022-09-28)

[Full Changelog](fvwmorg/fvwm3@1.0.4...1.0.5)

**Breaking changes:**

- Function parser rewrite & Repeat command deprecation [\#642](fvwmorg/fvwm3#642)
- MapRequest: don't fake map/unmap events [\#703](fvwmorg/fvwm3#703) ([ThomasAdam](https://github.com/ThomasAdam))
- Rewrite function parser and remove the Repeat command [\#643](fvwmorg/fvwm3#643) ([ThomasAdam](https://github.com/ThomasAdam))
- Update and cleanup SnapAttract code. [\#641](fvwmorg/fvwm3#641) ([somiaj](https://github.com/somiaj))
- Doc: split manpages into sections [\#637](fvwmorg/fvwm3#637) ([ThomasAdam](https://github.com/ThomasAdam))
- Remove Efence and Dmalloc support [\#635](fvwmorg/fvwm3#635) ([ThomasAdam](https://github.com/ThomasAdam))

**Implemented enhancements:**

- A better ManualPlacement that allows drawing the geometry of the new window. [\#674](fvwmorg/fvwm3#674)
- expand: add monitor.prev variable [\#699](fvwmorg/fvwm3#699) ([ThomasAdam](https://github.com/ThomasAdam))
- Add AnyScreen to conditional in IconManClick [\#696](fvwmorg/fvwm3#696) ([somiaj](https://github.com/somiaj))
- \_NET\_WM\_NAME: update to fvwm3 [\#609](fvwmorg/fvwm3#609) ([ThomasAdam](https://github.com/ThomasAdam))

**Fixed bugs:**

- Style \* Icon cause Fvwm3 stuck in loading when restart. [\#681](fvwmorg/fvwm3#681)
- Recaptured windows can have a negative offset away from the page they should be on [\#678](fvwmorg/fvwm3#678)
- VLC still decorates its transient window even when explicitly given the NakedTransient style [\#673](fvwmorg/fvwm3#673)
- configuring with `--disable-png` causes builds to fail [\#669](fvwmorg/fvwm3#669)
- Emoji in window titles make FvwmIconMan stop showing window names. [\#654](fvwmorg/fvwm3#654)
- Unable to initialize RandR [\#650](fvwmorg/fvwm3#650)
- PipeRead when called from a function cannot grab pointer [\#610](fvwmorg/fvwm3#610)
- Man Pages Cleanup [\#554](fvwmorg/fvwm3#554)
- Windows from various pages are moved to page 0 0 on fvwm3 restart [\#694](fvwmorg/fvwm3#694)
- Separator in menu gets focus [\#675](fvwmorg/fvwm3#675)
- Unshading a window with WindowShade function sometimes makes the window lose "true input focus". [\#671](fvwmorg/fvwm3#671)
- When configured with `--disable-xft` fvwm3 fails to build. [\#667](fvwmorg/fvwm3#667)
- my fvwm config does not work with recent chromium [\#663](fvwmorg/fvwm3#663)
- FvwmEvent event new\_desk gets triggered multiple times in multi-monitor shared setup [\#655](fvwmorg/fvwm3#655)
- Windows with style "PositionPlacement Center" split between monitors [\#648](fvwmorg/fvwm3#648)
- FVWM branch dv/pager-noaspect crashes with core dump [\#647](fvwmorg/fvwm3#647)
- SnapAttraction prefers wrong window [\#631](fvwmorg/fvwm3#631)
- FvwmPrompt is installed unstripped [\#618](fvwmorg/fvwm3#618)
- DesktopName fails to set desktop name under described circumstances [\#606](fvwmorg/fvwm3#606)
- FvwmEvent event monitor\_focus broken in FVWM3 1.0.4 [\#604](fvwmorg/fvwm3#604)
- Building FvwmPrompt disables FvwmConsole, but still installs manual page. [\#597](fvwmorg/fvwm3#597)
- Wait command in configuration file can cause unexpected issues with GeometryWindow. [\#590](fvwmorg/fvwm3#590)
- "GeometryWindow Hide" doesn't work [\#589](fvwmorg/fvwm3#589)
- Special characters \(umlauts\) are sometimes not displayed correctly in the window title [\#482](fvwmorg/fvwm3#482)
- FvwmEvent: handle previous\_monitor and no longer passthrough ID  [\#701](fvwmorg/fvwm3#701) ([ThomasAdam](https://github.com/ThomasAdam))
- doc: don't build FvwmConsole.1 if FvwmPrompt enabled [\#700](fvwmorg/fvwm3#700) ([ThomasAdam](https://github.com/ThomasAdam))
- DesktopConfiguration shared: keep windows in-situ [\#697](fvwmorg/fvwm3#697) ([ThomasAdam](https://github.com/ThomasAdam))
- desk\_add: fix starting desk/monitor [\#689](fvwmorg/fvwm3#689) ([ThomasAdam](https://github.com/ThomasAdam))
- shared: fix flagging of new\_desk [\#687](fvwmorg/fvwm3#687) ([ThomasAdam](https://github.com/ThomasAdam))
- Fix for lock recusion in handle\_all\_expose\(\) [\#683](fvwmorg/fvwm3#683) ([mherrb](https://github.com/mherrb))
- Asciidoc fixes [\#676](fvwmorg/fvwm3#676) ([topcat001](https://github.com/topcat001))
- grow: ignore transient windows [\#627](fvwmorg/fvwm3#627) ([ThomasAdam](https://github.com/ThomasAdam))
- MoveToScreen: fix NULL-dereference [\#605](fvwmorg/fvwm3#605) ([ThomasAdam](https://github.com/ThomasAdam))
- Bugfix: fvwm-menu-desktop --get-menus [\#593](fvwmorg/fvwm3#593) ([somiaj](https://github.com/somiaj))

**Closed issues:**

- Code Cleanup: Codacy issues list [\#107](fvwmorg/fvwm3#107)

**Merged pull requests:**

- avoid sprintf\(%n\) [\#653](fvwmorg/fvwm3#653) ([omar-polo](https://github.com/omar-polo))
- FvwmPrompt: add GOFLAGS to build stripped [\#619](fvwmorg/fvwm3#619) ([Zirias](https://github.com/Zirias))
- Wait: don't run until windows are captured [\#592](fvwmorg/fvwm3#592) ([ThomasAdam](https://github.com/ThomasAdam))
- CMD\_GeometryWindow: Move NULL check. [\#591](fvwmorg/fvwm3#591) ([somiaj](https://github.com/somiaj))
- cleanup: address warnings [\#705](fvwmorg/fvwm3#705) ([ThomasAdam](https://github.com/ThomasAdam))
- modconf: disable debug [\#698](fvwmorg/fvwm3#698) ([ThomasAdam](https://github.com/ThomasAdam))
- GotoDesk: avoid over-eager matching [\#695](fvwmorg/fvwm3#695) ([ThomasAdam](https://github.com/ThomasAdam))
- update\_fvwm\_monitor: cosmetic change [\#692](fvwmorg/fvwm3#692) ([ThomasAdam](https://github.com/ThomasAdam))
- menuitem: set selectable when not a separator [\#690](fvwmorg/fvwm3#690) ([ThomasAdam](https://github.com/ThomasAdam))
- Windowshade: explicitly set input focus [\#672](fvwmorg/fvwm3#672) ([ThomasAdam](https://github.com/ThomasAdam))
- FvwmPrompt: update core modules [\#665](fvwmorg/fvwm3#665) ([ThomasAdam](https://github.com/ThomasAdam))
- FvwmPrompt: update vendor deps [\#664](fvwmorg/fvwm3#664) ([ThomasAdam](https://github.com/ThomasAdam))
- Fix selectable flag for the Resize window operation menu item [\#656](fvwmorg/fvwm3#656) ([topcat001](https://github.com/topcat001))
- Fix ExitFunction [\#651](fvwmorg/fvwm3#651) ([pghvlaans](https://github.com/pghvlaans))
- DisplayPosition: fix segfault [\#645](fvwmorg/fvwm3#645) ([ThomasAdam](https://github.com/ThomasAdam))
- convert UPDATE\_FVWM\_SCREEN from macro to function [\#644](fvwmorg/fvwm3#644) ([ThomasAdam](https://github.com/ThomasAdam))
- ta/dv logfile [\#640](fvwmorg/fvwm3#640) ([ThomasAdam](https://github.com/ThomasAdam))
- Resize: fix resize bounds [\#638](fvwmorg/fvwm3#638) ([ThomasAdam](https://github.com/ThomasAdam))
- ta/dv2 [\#636](fvwmorg/fvwm3#636) ([ThomasAdam](https://github.com/ThomasAdam))
- ta/dv misc [\#634](fvwmorg/fvwm3#634) ([ThomasAdam](https://github.com/ThomasAdam))
- Reject out of range windows for Move and Resize commands. [\#633](fvwmorg/fvwm3#633) ([ThomasAdam](https://github.com/ThomasAdam))
- FVWMMFL: ignore SIGPIPE [\#632](fvwmorg/fvwm3#632) ([ThomasAdam](https://github.com/ThomasAdam))
- ta/dv ifdev [\#630](fvwmorg/fvwm3#630) ([ThomasAdam](https://github.com/ThomasAdam))
- ta/from dv [\#629](fvwmorg/fvwm3#629) ([ThomasAdam](https://github.com/ThomasAdam))
- DesktopName: don't duplicate entries with same name [\#607](fvwmorg/fvwm3#607) ([ThomasAdam](https://github.com/ThomasAdam))
- Patches from Debian [\#599](fvwmorg/fvwm3#599) ([ThomasAdam](https://github.com/ThomasAdam))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:bug Something's broken!
Projects
Status: Done
FVWM3
  
Done
Development

Successfully merging a pull request may close this issue.

4 participants