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

Zellij won't read config changes to locked mode #739

Closed
qballer opened this issue Sep 23, 2021 · 18 comments
Closed

Zellij won't read config changes to locked mode #739

qballer opened this issue Sep 23, 2021 · 18 comments
Labels
bug Something isn't working

Comments

@qballer
Copy link
Member

qballer commented Sep 23, 2021

I'm adding the alt options we have to the lock mode in order for them to be availble for me there. Changes aren't apply?
Side note: any way to refresh config without leaving zellij ?

Basic information
zellij --version: 0.0.17
tput lines:40
tput cols:156
uname -av or ver(Windows):Darwin C02FX1QMMD6R 20.6.0 Darwin Kernel Version 20.6.0: Wed Jun 23 00:26:31 PDT 2021; root:xnu-7195.141.2~5/RELEASE_X86_64 x86_64

List of programs you interact with as, PROGRAM --version: output cropped meaningful, for example:
alacritty --version: alacritty 0.9.0

Further information
Reproduction steps, noticable behavior, related issues etc
Screen Shot 2021-09-23 at 17 36 26

@qballer qballer added bug Something isn't working good first issue Good for newcomers hacktoberfest For the hacktoberfest month labels Sep 23, 2021
@imsnif imsnif changed the title Zellij won't read config changes Zellij won't read config changes to locked mode Sep 23, 2021
@imsnif
Copy link
Member

imsnif commented Sep 23, 2021

I think this is an issue of Zellij not reading the default configuration from ~/.config/zellij, because when I take this configuration and start zellij with zellij --config ~/.config/zellij it works. Maybe @a-kenji has an idea why?

@a-kenji
Copy link
Contributor

a-kenji commented Sep 23, 2021

~/.config/zellij/config.yaml
Should be the default configuration location.
What is the output you are getting when running

zellij setup --check

Side note: any way to refresh config without leaving zellij ?

Not yet! It's certainly something that is in our minds.

@qballer
Copy link
Member Author

qballer commented Sep 23, 2021

I've used config.yml and not config.yaml (dropping the a) which caused the issue

@imsnif
Copy link
Member

imsnif commented Sep 24, 2021

@a-kenji - thanks! Do you think we can accept both suffixes?

@qballer
Copy link
Member Author

qballer commented Sep 24, 2021

Also if we can avoid fixing it for now and let some hacktoberfest people get involved it would be awesome :)

@a-kenji
Copy link
Contributor

a-kenji commented Sep 24, 2021

Do you think we can accept both suffixes?

@imsnif
We could, but I think this idea has very little merit and just adds complexity and potentially confusion.

To quote from the documentation:

By default Zellij will look for config.yaml in the config directory.

We already support multiple config directories, in order to make it easier for the user as well as to comply with platform specific directories, as well as make it possible for distributions to use their system directories for further customization:

  1. $HOME/.config/zellij
  2. default config location (they differ on darwin and linux)
  3. system location (/etc/zellij), can be configured by a compile time flag by distributions

In that order.

That means we look for example for config files on linux in this order (excluding env vars and config options):

  1. $HOME/.config/zellij/config.yaml
  2. (same as above on linux)
  3. /etc/zellij/config.yaml

We already tell the user very clearly where the correct location is and even which location currently is loaded, try it out yourself:

mv ~/.config/zellij/ ~/.config/zellij-bak
zellij setup --check
mkdir ~/.config/zellij
zellij setup --check
zellij setup --dump-config > ~/.config/zellij/config.yaml
zellij setup --check

If we now add and ambiguous name for config files we now need to look at twice the amount of locations, the config files need to have an order. Which one has precedence? I think we are already flexible enough in supporting multiple directories. This could lead to problems in people sharing configurations. This is all implicit behaviour and I would like it to be as clear as possible.

So I would like to keep the name of the config file that we are loading to a single file, which name that is I don't really care. But I don't think multiple are a good way to go.

@imsnif
Copy link
Member

imsnif commented Sep 24, 2021

@a-kenji - fair enough. But notice that both myself and @qballer were totally stumped by why this wasn't working. I would guess we're not alone here. How about displaying a friendly error if Zellij finds a .yml file and not a .yaml file in those locations?

@a-kenji
Copy link
Contributor

a-kenji commented Sep 24, 2021

Maybe you didn't read the documentation thoroughly,
or maybe did not run zellij setup --check?

I would guess we're not alone here.

One is never alone.
I think we should then improve the documentation,
I now added a quickstart section that should work on darwin and linux systems.

How about displaying a friendly error if Zellij finds a .yml file and not a .yaml file in those locations?

I don't really like that as well. Because why stop at config.yml, why not at cnfig.yaml ,etc ... . What if I did mv config.yaml config.yaml.bak because I want to test something does that give me a friendly error then?
Where do we stop? Do we also give an error if we think the config directory has a typo? ~/.config/zelli/config.yaml

At the end this was just a typo.

@imsnif
Copy link
Member

imsnif commented Sep 24, 2021

From a user's perspective - don't you think it would be better if the app tells you what you're doing wrong rather than you hammering at the keyboard for hours until you notice a one letter difference in what you were typing and what's in the docs?

IMO we should not stop at all. Given infinite developer time I'd like the app to tell me exactly what I'm doing wrong at all times (the rust compiler is a really good example of this btw). I think we can totally catch all the things that are reasonably a mistake here. To your specific questions:

cnfig.yaml - should be an error (if it's inside the Zellij configuration folder) - probably everything that's a Levenshtein distance of 2 btw
config.yaml.bak - should not be an error
~/.config/zelli/config.yaml - should not be an error because that folder is not under our control and there's a small chance it will collide with other program's configuration folders (if someone decides to name their program Zelli for some reason :) )

IMO a good UX is having what the user expects to happen happen as much as reasonably possible. If a user fails to do something and we tell them they should read the docs more carefully, that's on us.

It's great that you added a quickstart section. Maybe that would have mitigated it. But where's the harm in giving an error (or at least a warning) if you have a config.yml in a configuration folder we control?

@a-kenji
Copy link
Contributor

a-kenji commented Sep 24, 2021

Did you try that?
If you haven't please do.
If you have then I apologize, but it just seems to me that you did not.

We already tell the user very clearly where the correct location is and even which location currently is loaded, try it out yourself:

mv ~/.config/zellij/ ~/.config/zellij-bak
zellij setup --check
mkdir ~/.config/zellij
zellij setup --check
zellij setup --dump-config > ~/.config/zellij/config.yaml
zellij setup --check

And also you can try that:

mv ~/.config/zellij/ ~/.config/zellij-bak
mkdir ~/.config/zellij
zellij setup --check
zellij setup --dump-config > ~/.config/zellij/config.yml
zellij setup --check

@imsnif
Copy link
Member

imsnif commented Sep 24, 2021

@a-kenji - I'm not trying to be right here. I'm really just trying to create the best experience for potential users facing this same issue. If the setup told me that this configuration file would not be loaded, I still wouldn't necessarily understand what the issue is. I'd probably mostly be frustrated about why it's not loading my configuration file "which is clearly what is written in the docs!!111oneone" :) (a one letter difference in the middle of a word is really hard to spot if you don't know what you're looking for and don't have the context of a maintainer).

If the setup can give you a warning about possible mistakes like we did above - that can also be a great solution. But if I don't know the setup command exists, how can I find out about it in the context of trying to get my configuration file to work?

@a-kenji
Copy link
Contributor

a-kenji commented Sep 24, 2021

I'm not trying to be right here. I'm really just trying to create the best experience for potential users facing this same issue.

I agree, and I see that you are doing it. But I see myself as a user too and I try the same.
We both want to improve the experience for the user here.

IMO a good UX is having what the user expects to happen happen as much as reasonably possible.

I agree, but It would frustrate me, if I had a file called differently in the folder and zellij would throw an error because of that. That would be implicit and hard to understand for me I believe.
I think we can empower the user in a different way.

If the setup told me that this configuration file would not be loaded, I still wouldn't necessarily understand what the issue is. I'd probably mostly be frustrated about why it's not loading my configuration file "which is clearly what is written in the docs!!111oneone" :) (a one letter difference in the middle of a word is really hard to spot if you don't know what you're looking for and don't have the context of a maintainer).

The setup does tell you if the expected file is not there:

[CONFIG FILE]: "$HOME/.config/zellij/config.yaml"
[CONFIG ERROR]: IoError: No such file or directory (os error 2), File: $HOME/.config/zellij/config.yaml

But if I don't know the setup command exists, how can I find out about it in the context of trying to get my configuration file to work?

That is a very good point, I honestly thought it was so clearly visible that it is hard to miss.
I will try to make it more prominent.

If I want to start zellij I want to have as little friction as possible, especially implicit friction.
If someone wants to check their configuration they should be able to run zellij setup --check.
Here we can reasonably expect that the user wants to check their configuration and maybe
look for different files too in the config directory that are similarly named?
This will be especially important in my opinion later if we integrate layouts and config files more.

Back to the topic, to make the setup command more prominent I would rather
write a small section about it in the default config file.

And something I actually have been wanting to do for a while:

Add to the goodbye screen:

Instead of :

Bye from Zellij!

A small blurb, that says the default configuration was loaded,
if it was loaded and instructions on how to get set up:

Loaded default configuration.
To show default config: `zellij setup --dump-config`
To check your config: `zellij setup --check`
Bye from Zellij!

@qballer
Copy link
Member Author

qballer commented Sep 24, 2021

As for @imsnif's point a very common example:
Screen Shot 2021-09-24 at 21 57 46

I almost wanted zellij to generate a configuration file in ~/.config/zellij/config.yaml in case no configuration exists and it has to go to the default configuration embedded in the code. The config is really clear once you see it. It's a shame not show it the users. I would also print a message that zellij created a config as none was found. Doing that I believe saves the user time to lookup the docs as it is self explanatory. I do see down sides to this approach in the form of -"oh, that mesages that was printed in the first time zellij started. I ignored that."

While not the main conversation. I also believe supporting both yaml and yml is the best way. It is not the same as a normal edit distance as there is much open debate about yaml vs yml in the interweb. This is totally not needed if zellij does what I suggested before that.

I didn't know about zellij setup --check and once I did know about it everything worked out really nicely. I can go about zellijing my merry way :)

Thanks for putting so much thought and effort to the UX, it is very important in my mind and we should do what helps the user use zellij. Like we did in other places.

@imsnif
Copy link
Member

imsnif commented Sep 24, 2021

Right. @a-kenji - I understand you not wanting to have Zellij open similarly named files by default or throw an error because they exist. I get it. That'll for sure be bad UX.

While I think the setup command is really cool - it definitely is something you need to know about. No matter how prominent it is in the docs, if you're looking for something else you are very likely to glance over it. Even if it's literally one line above the thing you were looking for. That's how we as devs browse documentation - there's no avoiding that. :)

What do you think - since this is a hot-button issue - that we look just at this one case and try to solve it?

IMO, the main issue here is the "yml" vs. "yaml" endings. Both of which are acceptable and common endings. Here's an example of how "docker-compose" solves it:

$ ls | grep docker-compose
docker-compose.yaml
docker-compose.yml

$ docker-compose up -d
WARNING: Found multiple config files with supported names: docker-compose.yml, docker-compose.yaml
WARNING: Using docker-compose.yml

For our case, this might be a little problematic because Zellij clears the terminal as soon as it starts. We could build a warning infrastructure that would sleep for a few seconds while displaying warnings (if they are there) and then start the app. But this seems like a lot of work for just one instance that we don't know if will be the first of many or not.

How about we throw an error just for this case of yml vs. yaml? No Levenstein distance or anything. There will not be a slippery slope here. Just this one. Instead of supporting both like other tools (or at least like docker-compose), we'll just have this behaviour that will save users from this case? Clearly, users who don't know about setup hit this. Two maintainers hit this - one of whom did know about setup but wasn't thinking of using it (honestly, it was so obvious to me that the problem is elsewhere that I didn't even think of trying - us Humans are fallible and sometimes need to be spoon-fed... or at least I am :) )

@a-kenji
Copy link
Contributor

a-kenji commented Sep 24, 2021

For our case, this might be a little problematic because Zellij clears the terminal as soon as it starts. We could build a warning infrastructure that would sleep for a few seconds while displaying warnings (if they are there) and then start the app. But this seems like a lot of work for just one instance that we don't know if will be the first of many or not.

This would be very easy to implement. All our config and layout errors are handled that way.

How about we throw an error just for this case of yml vs. yaml? No Levenstein distance or anything. There will not be a slippery slope here. Just this one. Instead of supporting both like other tools (or at least like docker-compose), we'll just have this behaviour that will save users from this case?

I am moderately OK with this, if I understand you correctly we want to error, if there is a config.yml file existing in the config directory?

@imsnif
Copy link
Member

imsnif commented Sep 24, 2021

This would be very easy to implement. All our config and layout errors are handled that way.

I have this feeling that it will cause some issues with timings and such, but maybe I'm wrong. For sure this is a better way if you think it's easy to implement (I was kind of hoping to make this one of the issues for hacktoberfest).

I am moderately OK with this, if I understand you correctly we want to error, if there is a config.yml file existing in the config directory?

My preference is that we support both and error if there's more than one. But I'm also okay with just erroring for config.yml. How can we change this so that you are more than moderately OK with it?

@a-kenji
Copy link
Contributor

a-kenji commented Sep 24, 2021

I have this feeling that it will cause some issues with timings and such, but maybe I'm wrong. For sure this is a better way if you think it's easy to implement (I was kind of hoping to make this one of the issues for hacktoberfest).

Try it out yourself. On main put this into a file:

---
tabs:
  - name: hello
    parts:
      - direction: Vertical
      - direction: Vertical
        name: the parts section can't be named yet

zellij --layout-path [PATH-TO-THE-FILE]

All one needs to do is define an error struct, impl the error boilerplate and then return the error on deserialization when we encounter the file.

How can we change this so that you are more than moderately OK with it?

Decide on one, either .yml, or .yaml. I don't care. I don't like the potential ambiguity it brings.
I think this is a bad experience. What should I then do if I want both a config.yaml and a config.yml file in my config directory. It's OK, I don't need to feel good about every decision.

@imsnif imsnif removed good first issue Good for newcomers hacktoberfest For the hacktoberfest month labels Sep 25, 2021
@a-kenji
Copy link
Contributor

a-kenji commented Nov 23, 2021

Closing this for now, hopefully #771 will address this.

@a-kenji a-kenji closed this as completed Nov 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants