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

Allow activating workspaces without making them the default everywhere #1417

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

hco
Copy link
Sponsor

@hco hco commented Nov 29, 2023

Hey there 👋
While I love the workspaces feature, I don’t want it to always be the default.
This way you can disable that workspaces is the default filter if a workspace is found.

This is the first time ever I wrote rust code, so this might be really wrong. Let me know if I can improve something!

PS: Thanks for Atuin and all the great work around it, I really like it.

While I love the feature, I don’t want it to always be the default.

This way you can disable that workspaces are the default filter if a workspace is found.
Copy link

vercel bot commented Nov 29, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
atuin-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 29, 2023 4:55pm

@arcuru
Copy link
Sponsor Contributor

arcuru commented Nov 29, 2023

I think this is a good idea, but I'd suggest an alternate approach.

workspaces is just a filter mode that doesn't always apply, so we should expose it to the user as a filter mode. It should always show up in the filter list if it is a valid filter (i.e. if we're in a workspace), but not as the default unless the user has set it.

We should just have filter_mode take a list of filter modes. So if a user wanted to use workspaces they should set that to "workspace, global", or even a list ["workspace" "global"]. That way it will be easier to add more optional filter modes in the future as well.

Even better, maybe just allow the user to set a list of all the filter modes that they want to see, and in what order.

@hco
Copy link
Sponsor Author

hco commented Nov 29, 2023

I love your idea. I can have a look if I can make this work.

That means you'd rather not merge this PR the way it is, right?

Apart from the feature going in the wrong direction: Have you looked if the code if that's okay in general? As this is my first time writing rust code anything I should do differently in a next iteration would be great to hear as early as possible 😊

@arcuru
Copy link
Sponsor Contributor

arcuru commented Nov 29, 2023

First, please don't make major changes until you get an opinion from ellie or conrad. I've contributed here but am not a maintainer, so I was just expressing my opinion :) I was just a little worried about adding several more flags to the config file so I wanted to propose something a little cleaner.

Second, I think my suggestion would require a larger change than this but it shouldn't be too bad. It'd mainly require some changes in how filter_mode itself is handled/parsed. It does depend on what they decide to do for this feature but I'm sure you could handle it.

The structure of your code here looks fine to me, and it seems like you understand how this code is working as well. I'm confident you'd have no trouble implement a different approach.

@hco
Copy link
Sponsor Author

hco commented Nov 29, 2023

Thanks for the input. So the best thing right now would be to wait for feedback from ellie or conrad?

@ellie
Copy link
Member

ellie commented Dec 2, 2023

We should just have filter_mode take a list of filter modes. So if a user wanted to use workspaces they should set that to "workspace, global", or even a list ["workspace" "global"]. That way it will be easier to add more optional filter modes in the future as well.

Sounds like a good idea to me! We'd need to ensure that there's a default in the case where workspaces = true, but the filter modes haven't been customized just yet, but otherwise I think this would be the best approach

Thanks for the work here @hco, do let us know if you need any help! And thanks @arcuru for your feedback and suggestions 🙏 You're correct in saying that I'd rather avoid a whole bunch of new flags, we're getting pretty heavy on those already

dasJ added a commit to dasJ/atuin that referenced this pull request Feb 13, 2024
dasJ added a commit to dasJ/atuin that referenced this pull request Feb 14, 2024
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