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

fix: crash when a script menu update renders selected index unavailable #591

Merged
merged 1 commit into from
Jul 26, 2023

Conversation

po5
Copy link
Contributor

@po5 po5 commented Jul 25, 2023

When a script-defined menu gets updated with fewer items than it previously had, the user's selected index doesn't get clamped or reset, leading to this crash when pressing Enter:

[uosc] 
[uosc] stack traceback:
[uosc]  /home/eva/.config/mpv/scripts/uosc/elements/Menu.lua:499: in function 'open_selected_item'
[uosc]  /home/eva/.config/mpv/scripts/uosc/elements/Menu.lua:514: in function </home/eva/.config/mpv/scripts/uosc/elements/Menu.lua:514>
[uosc]  (...tail calls...)
[uosc]  /home/eva/.config/mpv/scripts/uosc/elements/Menu.lua:678: in function 'fn'
[uosc]  mp.defaults:230: in function 'fn'
[uosc]  mp.defaults:65: in function 'handler'
[uosc]  mp.defaults:380: in function 'handler'
[uosc]  mp.defaults:510: in function 'call_event_handlers'
[uosc]  mp.defaults:552: in function 'dispatch_events'
[uosc]  mp.defaults:503: in function <mp.defaults:502>
[uosc]  [C]: in ?
[uosc]  [C]: in ?
[uosc] Lua error: /home/eva/.config/mpv/scripts/uosc/elements/Menu.lua:499: attempt to index local 'item' (a nil value)

I'm not sure if Menu:update_dimensions() is the proper place to update the selected index, but it fixes the issue.
Ran into this with memo, switching from the 2nd page of results back to the first, if you need a real example of it.

@christoph-heinrich
Copy link
Contributor

Menu:update() is a more appropriate place for that imo.

@po5
Copy link
Contributor Author

po5 commented Jul 25, 2023

I tried that before and it doesn't fix the crash.

@po5
Copy link
Contributor Author

po5 commented Jul 26, 2023

Done.

@tomasklaen
Copy link
Owner

Do a:

menu.selected_index = #menu.items > 0 and clamp(1, menu.selected_index, #menu.items) or nil

@po5 po5 force-pushed the updated-menu-oob-selected-index branch from 02a934e to ba884af Compare July 26, 2023 12:54
@po5
Copy link
Contributor Author

po5 commented Jul 26, 2023

(:

@tomasklaen
Copy link
Owner

No no, the condition was fine, since this shouldn't run when menu.selected_index is nil. I don't think clamp() handles nils.

@po5 po5 force-pushed the updated-menu-oob-selected-index branch from ba884af to b1cdc5a Compare July 26, 2023 15:08
@po5
Copy link
Contributor Author

po5 commented Jul 26, 2023

Should be good now.

@tomasklaen tomasklaen merged commit e783ad1 into tomasklaen:main Jul 26, 2023
tam1m pushed a commit to tam1m/uosc that referenced this pull request Oct 2, 2023
…le (tomasklaen#591)

Co-authored-by: Christoph Heinrich <christoph.heinrich@student.tugraz.at>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants