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

Mapping filter control with dedicated Gtk.ListBox filter manager #628

Open
wants to merge 23 commits into
base: beta
Choose a base branch
from

Conversation

ubunatic
Copy link
Contributor

This is a followup on #621. It adds

  • a generic list manager for Gtk.ListBoxes to show and hide rows
  • a mapping filter control and the related UI elements

See the screens in #621

@jonasBoss
Copy link
Collaborator

using a GtkSearchEntry gives us the clear button for free.
image

@ubunatic
Copy link
Contributor Author

Using GtkSearchEntry now.

P.S.: Ideally I would want to have sth. like this:
image

This is the VSCode search and I would love to have this on any list in any UI.

But one step at a time ;)

@ubunatic ubunatic force-pushed the feature/mapping-filter-generic branch from c878327 to d06dd8d Compare February 21, 2023 14:08
Copy link
Owner

@sezanzeb sezanzeb left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your contributions! :)

A few points/thoughts from my side:

  1. I would change the gui like this:

image

See https://github.com/sezanzeb/input-remapper/tree/628-gui-suggestion for the glade file that produces this

  1. I would like the search to also search through the selected target, the programmed key/macro and searching for ABS_X should find analog output that is set to ABS_X. I would be totally fine with having this in a separate future PR.

  2. Is the case sensitive search toggle really necessary? I'm not so sure that I would ever need a case-sensitive search here.

4.I think the toggle did not switch to the active state when clicking it, or is that just happening on my end due to the theme or something? EDIT: It was the theme

inputremapper/gui/components/gtkext/listbox_filter.py Outdated Show resolved Hide resolved
tests/integration/test_gui.py Outdated Show resolved Hide resolved
@ubunatic
Copy link
Contributor Author

  1. ✅ merged your proposal, I was looking for this magic linked style but could find it. Thx!
  2. Separate PR is better. This one gets bigger and bigger
  3. I would use it to search for generated text ("Alt L") vs. my own custom text ("ALT"). I have a lot of mappings now. And event the search is not enough actually. I would want a searchable tree or some other kind of grouping of things beyond being sorted A-Z. Maybe I would even group them manually. But for now the search is fine. Better than just scrolling. Let's start small.

@sezanzeb
Copy link
Owner

sezanzeb commented Feb 22, 2023

allright

I found the "linked" class when I used the gtk inspector on quod libet, it has a small dropdown button to the right of its search.

@ubunatic
Copy link
Contributor Author

I also added support for an explicit GTK_PATH in this PR now. This allows me to start the local development app with:

GTK_PATH=$PWD PYTHONPATH=$PWD bin/input-remapper-gtk -R

so that it will load the latest local files from ./data.

I test my changes this way. First I start the app. Then I press ESC on the pkexec dialog. And then I try my changes in the GUI.

Copy link
Owner

@sezanzeb sezanzeb left a comment

Choose a reason for hiding this comment

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

Thanks. -R sounds handy for development.

Lets just move those two comments into docstrings.

Is this ready to merge then?

inputremapper/gui/components/gtkext/listbox_filter.py Outdated Show resolved Hide resolved
Co-authored-by: Tobi <to.213692@protonmail.ch>
@ubunatic
Copy link
Contributor Author

Ready to be merged from my side.

P.S. @sezanzeb I have sent you an email ;)

Copy link
Owner

@sezanzeb sezanzeb left a comment

Choose a reason for hiding this comment

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

I want to make the 2.0 release soon.

Do you have any plans regarding the search looking through the outputs as well already? If not, I'd probably merge this after 2.0.

raise Exception(f"Failed to pkexec the reader-service, code {exit_code}")
ex = Exception(f"Failed to pkexec the reader-service, code {exit_code}")
if ingore_errors:
logger.warn(ex)
Copy link
Owner

Choose a reason for hiding this comment

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

Some people might have very permissive setups by being member of the "device" user group or something. In that case, it might make sense to at least try to run input-remapper-control --command start-reader-service{debug} without pkexec as a regular user.

Copy link
Contributor Author

@ubunatic ubunatic Feb 23, 2023

Choose a reason for hiding this comment

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

Actually, I would like to move better pkexec handling to another PR and actually avoid having a need for a -R flag (or similar).

Here is what I would be looking for in a later change. But this fails many tests, so I guess the story is bigger than I thought.

Copy link
Owner

@sezanzeb sezanzeb Feb 24, 2023

Choose a reason for hiding this comment

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

the "device" user group or something

I think it's called "input"


If the -R flag might be deprecated soon already, I wouldn't add it now

I think users being in the input group is the exception. I don't know right now how evdev behaves when an unprivileged user tries to use InputDevice objects and such. I guess you'd have to look out for any permission error and then exit. I would also use a special exit code for this >= 3 and then check for that one specifically before deciding to use pkexec.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I will remove the -R now but will leave an undocumented env var for development. As said, let's discuss better privilege handling separately.

@ubunatic
Copy link
Contributor Author

ubunatic commented Feb 23, 2023

Do you have any plans regarding the search looking through the outputs as well already? If not, I'd probably merge this after 2.0.

Not sure this a good idea without more entry controls. The names in the list are defined by the input only; unless renamed manually. We would need more buttons, I guess:
image

But then the UI seems to get more complicated. The user would need to know that he is searching through text that is not visible in the list; only the first output will be shown on the right. Let's have a discussion about better filters later.

If you want this in a 2.0, I would say we stay minimal with the change:

  • remove the pkexec stuff (maybe replaced by development-only env var for now)
  • keep the filter on visible input text only

I can do these changes tomorrow, after that I am off for the weekend and will also have less time next week.

@ubunatic ubunatic force-pushed the feature/mapping-filter-generic branch from 13919a5 to 1f9cf3f Compare February 24, 2023 09:53
@jonasBoss
Copy link
Collaborator

If you want this in a 2.0, I would say we stay minimal with the change:

  • remove the pkexec stuff (maybe replaced by development-only env var for now)
  • keep the filter on visible input text only

I think for now this makes the most sense.


Searching through the whole mapping would require us to do the filtering in the data manager. I think that can be done by filtering inside the publish_mapping() method. The controller would set the filter in the data_manager, and then call publish_mapping whenever the filter changes.
I don't think we need more buttons for that we could just always search the whole mapping. I can not imagine that someone needs the extra granularity. If we want an advanced search, that should be done inside a dedicated menu. So we keep the main UI simple.

@ubunatic ubunatic force-pushed the feature/mapping-filter-generic branch from 1f9cf3f to fe9dd37 Compare February 24, 2023 14:45
@ubunatic
Copy link
Contributor Author

If we want an advanced search, that should be done inside a dedicated menu. So we keep the main UI simple.

agreed.

I think I am done with this PR. If there are smaller issues, I can address them next week. But I will not have time for major changes in the next days.

@sezanzeb
Copy link
Owner

sezanzeb commented Feb 25, 2023

Finding something I'm looking for is easy when I can see it.

The output is not visible unless I click on a mapping.

Finding something that maps to a specific key is impossible unless I click on every mapping to look through every one of them.

I think a search only makes sense when it also filters through the output. And I wouldn't make an extra window for it.

@sezanzeb
Copy link
Owner

If you want to make an advanced search, then the advanced search would be tweakable to only look for specific things, while the regular search searches everything.

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.

None yet

3 participants