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

feat: palette menus #652

Merged
merged 8 commits into from
Sep 29, 2023
Merged

feat: palette menus #652

merged 8 commits into from
Sep 29, 2023

Conversation

tomasklaen
Copy link
Owner

@tomasklaen tomasklaen commented Sep 28, 2023

Each (sub)menu can now enable palette mode by setting its palette property to true.

In this mode:

  • search input is always visible, doesn't have to be enabled, and can't be disabled
  • title is used as input placeholder while search is empty

Any suggestions/bugs/comments?

Each (sub)menu can now enable palette mode by setting its `palette` property to `true`.

In this mode:

- search input is always visible, doesn't have to be enabled, and can't be disabled
- `title` is used as input placeholder while search is empty
scripts/uosc/elements/Menu.lua Outdated Show resolved Hide resolved
scripts/uosc/elements/Menu.lua Show resolved Hide resolved
Comment on lines 219 to 227
for _, menu in ipairs(self.all) do
if menu.palette and not menu.search then
update_dimensions_again = true
self:search_init(menu)
elseif not menu.palette and menu.search and menu.search.query == '' then
update_dimensions_again = true
menu.search = nil
end
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can't that be done before the dimensions update? Then you wouldn't require update_dimensions_again.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We update before and after because search_inits need the initial un-searched menu's position and scroll state to save on the search.source table.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like it but I also can't think of something better :)

@christoph-heinrich
Copy link
Contributor

What's the point of that palette mode? Starting a search isn't that big of a deal for users.

@tomasklaen
Copy link
Owner Author

tomasklaen commented Sep 29, 2023

The point of palette mode is to make palette menus :) Like ctrl+shift+p in vscode.

For example I have a menu-palette command in the pipe when this is merged. It'll display a flattened version of user defined menu to instantly search through:

menu-palette

I also want inputs-palette, that will display every binding defined in input.conf with command as title, and comment as hint.

It'll also be useful for implementing stuff like subtitle downloader, where there's just an empty list initially with a search input at the top pre-populated with current file name.

I'd expect it to look something like:

image

@tomasklaen
Copy link
Owner Author

tomasklaen commented Sep 29, 2023

search_query_update() will cause a search to be executed, which is probably not what we want here? I'm not even sure tbh.

I don't really know if I want it to be executed or not. I'm going with yes atm, but not sure about it. The use case is of course the subtitle downloader, where you make a menu with on_search callback, so setting initial_query will just prepopulate but not execute until ctrl+enter is pressed, effectively making it a search suggestion. It just feels useful to be able to populate initial queries for menu implementers.

I thought about making it just a search_suggestion (fills the query but doesn't execute search), and if we want scripts to execute searches from their end, implement something like menu-search <type> <id> <query> message. But what would be the behavior of search suggestion in instant internal search? It feels weird for it to not search and have an unfiltered menu that doesn't match query.

@christoph-heinrich
Copy link
Contributor

christoph-heinrich commented Sep 29, 2023

It just feels useful to be able to populate initial queries for menu implementers.

Agree

implement something like menu-search <type> <id> <query> message.

Isn't that the point of on_search? That's how the search for memo is currently implemented. Scripts can put whatever identifying information they need in addition to the query into their specified callback, and then update the menu with the search results using menu-update.

But what would be the behavior of search suggestion in instant internal search? It feels weird for it to not search and have an unfiltered menu that doesn't match query.

Scripts that want to provide a prefilled search field with the corresponding search result while providing on_search can simply set those items when opening the menu. They already know what the search query is, so no need for that callback.
If their search is too slow they can open the menu with some placeholder item and immediately after call whatever function would be called via on_search to execute a search and update the items.

But when the internal search is used we need to execute a search.

@tomasklaen
Copy link
Owner Author

tomasklaen commented Sep 29, 2023

Isn't that the point of on_search

True :)


Renamed it to search_suggestion, but kept the behavior (executes internal search, but waits for ctrl+enter on external). It feels like it conveys what the purpose of it is better.

@tomasklaen tomasklaen merged commit 9b251ef into main Sep 29, 2023
@christoph-heinrich
Copy link
Contributor

So you're fine with external searches calling on_search right after the menu was opened if they have a search_suggestion and search_debounce ~= 'submit' ?

@tomasklaen
Copy link
Owner Author

No I'll fix it 😶.

tam1m pushed a commit to tam1m/uosc that referenced this pull request Oct 2, 2023
Each (sub)menu can now enable palette mode by setting its `palette` property to `true`.

In this mode:

- search input is always visible, doesn't have to be enabled, and can't be disabled
- `title` is used as input placeholder while search is empty
- `search_suggestion` can be used to pre-populate search input with an initial query
tam1m pushed a commit to tam1m/uosc that referenced this pull request Oct 2, 2023
@tomasklaen tomasklaen deleted the palette branch October 9, 2023 12:02
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.

2 participants