Fix missed rendering regressions on Wayland (fixes --grab flag) #273
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Cloudef
reviewed
Jun 29, 2022
client/common/common.c
Outdated
| @@ -516,6 +516,7 @@ menu_with_options(struct client *client) | |||
| // bm_menu_grab_keyboard(menu, true); | |||
| bm_menu_render(menu); | |||
| bm_menu_set_filter(menu, NULL); | |||
| menu->dirty = true; // Ensure a redraw on the first call to bm_menu_render after the items are loaded | |||
There was a problem hiding this comment.
The dirty is not considered public API. I think you should set this in this function instead
Line 175 in 84bccc0
There was a problem hiding this comment.
Done. Note that most bm_menu_set_* are missing the dirtiness check, which is what initially led me to try to just set the dirty flag and call it a day, but I agree that the ideal approach going forward should be to set it on all bm_menu_set_* functions that cause a change on the UI.
Currently, on the Wayland backend, the following command:
{ echo one; sleep 1; echo two; } | BEMENU_BACKEND=wayland bemenu --grab
Will keep showing the 'Loading...' text even after all items are available.
The items will only be shown after some input, e.g. a key press, happens.
This was a regression introduced by 5a09570
(versions 0.6.5+). A dirty flag was added to avoid unnecessary redraws,
however, this flag is not reset between the renders before/after the items are
loaded, so the bm_menu_render call after the items are loaded is ignored
(on Wayland, which is the only renderer that currently uses the dirty flag).
Fix the issue by setting the dirty flag when the filter changes, so it will be
reset when changing from "Loading..." to empty and cause a redraw.
Despite the fix in the previous commit (3d7e47c), the following command: { echo one; echo two; } | BEMENU_BACKEND=wayland bemenu --grab Will likely still show the 'Loading...' text after all items are available. A related problem (also on Wayland) is that when pressing two keys (for the filter) almost simultaneously, sometimes only one of the keys will be rendered. The other key will only be shown on the next render. For example: - Filter shows "s" - User presses the "o" and "n" keys simultaneously - Filter shows "so" ["n" appears to have been lost] - User presses the "g" key - Filter shows "song" [now the "n" has been rendered] Both problems have the same root cause: If two events that cause a render happen "close enough" to each other, often the second event will not cause a render. As far as I can tell, the cause is that the "dirty && render_pending" render check should not check the dirty flag at all, just "render_pending". With "dirty && render_pending", if two events happen in close succession: - On the first event, generally dirty==true, render_pending==true, a render will happen, the frame callback will be set, and both flags will be cleared. - For the second event, generally dirty==true and render_pending==false. dirty will be unset and nothing will happen. - When the frame triggers, render_pending is set, but no render will happen because dirty==false With just "render_pending" (the change in this commit): - On the first event, generally dirty==true, render_pending==false, the frame callback will be set, and dirty will be cleared. - For the second event, generally dirty==true and render_pending==false. dirty will be unset and nothing will happen. - When the frame triggers, render_pending is set, then a render will be done.
joanbm
commented
Jun 29, 2022
| free(menu->filter); | ||
| menu->filter_size = (filter ? strlen(filter) : 0); | ||
| menu->filter = (menu->filter_size > 0 ? bm_strdup(filter) : NULL); | ||
| menu->curses_cursor = (menu->filter ? bm_utf8_string_screen_width(menu->filter) : 0); | ||
| menu->dirty |= (menu->cursor != menu->filter_size); |
There was a problem hiding this comment.
(This is not necessary to fix the issue at hand, but just not to leave the dirtiness check incomplete).
|
Thanks merged |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR contains two closely related regression fixes (introduced by commit 5a09570 and released in versions 0.6.5+), which affect exclusively the Wayland backend.
Both are due to some missed edge cases when managing the
dirtyflag that was introduced in that commit, and for the most part just cause bemenu not to re-render after the items are loaded when using the--grab, so bemenu stays at "Loading..." until an unrelated event happens (e.g. a key press) that causes an actual re-render.The issue fixed by the first commit can be reproduced with:
{ echo one; sleep 1; echo two; } | BEMENU_BACKEND=wayland bemenu --grabAnd causes bemenu to pause at "Loading..." almost 100% of the time.
The issue fixed by the second commit can be reproduced with:
{ echo one; echo two; } | BEMENU_BACKEND=wayland bemenu --grabAnd causes bemenu to sometimes (~15% on my PC) pause at "Loading...". Reproduces more often when having some background activity.