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

Add support for Ilia #21

Closed
wants to merge 4 commits into from
Closed

Add support for Ilia #21

wants to merge 4 commits into from

Conversation

jc00ke
Copy link
Contributor

@jc00ke jc00ke commented Aug 8, 2023

Ilia is a desktop executor for Regolith. Thanks for your consideration!

jc00ke and others added 4 commits August 8, 2023 11:14
Ilia is used by Regolith Linux (Ubuntu+i3/sway).
With the growing amount of picker tools supported out of the box
it seems more practical to have them together in a simple list
that can grow and through which we can always iterate instead
of a cumbersome growing if-else chain.

This commit moves the pick tool selection to be stored in
a bash hash map instead.
Copy link
Owner

@marty-oehme marty-oehme left a comment

Choose a reason for hiding this comment

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

Hi Jesse, sorry for being a little slow on the response.

I have taken the liberty of changing the code around a little;
with a look to possibly supporting even more picking tools in the future I moved the current selection into a hash map which we iterate through.

Can you double-check with the new code changes that everything works well?
If you think everything looks good I think we are ready to merge.

Thanks for the PR!

["wofi"]="wofi -p 🔍 -i --show dmenu"
["rofi"]="rofi -p 🔍 -i -dmenu --kb-custom-1 "Alt+1" --kb-custom-2 "Alt+2""
["dmenu"]="dmenu -p 🔍 -i -l 20"
["ilia"]="ilia -n -p textlist -l 🔍"
Copy link
Owner

Choose a reason for hiding this comment

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

From the ilia documentation I gathered that we can display our little spy-glass with the -l option but I have no way of testing this locally without access to ilia.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

2023-08-25_15-43

🔍 is now displayed at the bottom, but I now think it's not quite what you would expect it to. Here are some options:

ilia -n -p textlist -l "Emoji 🔍"
2023-08-25_16-18

ilia -n -p textlist -l "🔍 Emoji"
2023-08-25_16-19

ilia -n -p textlist -l "Emoji" -i desktop-magnifier (from the GTK theme)
2023-08-25_16-21

ilia -n -p textlist -l "🔍 Emoji" -i ""
2023-08-25_16-22

@kgilmer I might be missing something... is there a way to completely disable the icon (like with Help) or use a character and not a GTK icon?

Copy link
Owner

Choose a reason for hiding this comment

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

Ah, I knew I misunderstood something with the label. If there is no option to completely disable the icon I think I am fine with either of the last two choices; perhaps the GTK-icon actually makes the most sense since we are now in GTK-world. Thanks for looking into it!

@@ -20,6 +20,14 @@ bm_private_mode=${BEMOJI_PRIVATE_MODE:-false}
# Do not sort results
bm_ignore_recent=${BEMOJI_IGNORE_RECENT:-false}

declare -A default_pickers=(
Copy link
Owner

Choose a reason for hiding this comment

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

I am not entirely sure which way would be the best to sort pickers:
I thought about going from most well known (e.g. most GH stars) to least well known, but that seems counter-productive since most people will often want to use the most 'obscure' picker they have installed I would think?
However, going the reverse way also seems counter-intuitive.

I have left it as is for now and just appended ilia, let me know if you have a better idea or some input on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about passing a picker via flag? I think by default whatever order is fine, but as for which one to actually execute, it makes most sense to me to allow the user to pass that in, and an argument to the script seems cleanest, even over an ENV var.

Copy link
Owner

Choose a reason for hiding this comment

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

I think you are right and we can probably actually combine both worlds by letting users choose a default picker or supply a custom one through the same interface. I'll open a new issue for that since it goes a bit beyond this PR.

Copy link
Owner

Choose a reason for hiding this comment

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

And just as a last note to myself: the order of initializing the hash map does not even matter in bash, it's always sorted based on hash in the end.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like tessen does something similar, FIFO-ish.

@jc00ke
Copy link
Contributor Author

jc00ke commented Aug 25, 2023

The rewrite (welcome by me!) works locally, however the icon is a little wonky, but I addressed that in a comment above.

marty-oehme pushed a commit that referenced this pull request Sep 14, 2023
Ilia is used by Regolith Linux (Ubuntu+i3/sway).

Ilia has a slightly different presentation and prompt setup than other
pickers, and it makes more sense to use the given GTK icon instead of
the emoji character since that is what it seems to support.

Nicely explained by @jc00ke here:
#21 (comment)

With the growing amount of picker tools supported out of the box
it seems more practical to have them together in a simple list
that can grow and through which we can always iterate instead
of a cumbersome growing if-else chain.
This commit also refactors the pick tool selection to be stored in
a bash hash map instead.
@marty-oehme
Copy link
Owner

Merged as 74af60c.

I used your suggestion of GTK icon for the label.
I realize this was not the biggest commit but thanks for your contribution, all the help and patience!

@jc00ke
Copy link
Contributor Author

jc00ke commented Sep 15, 2023

I'm happy to contribute any way that's helpful! Thanks for adding ilia.

@jc00ke jc00ke deleted the ilia branch September 15, 2023 04:27
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

2 participants