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

Issue 1321: update amethyst.sample.yml include all defaults #1326

Merged

Conversation

wley3337
Copy link
Contributor

@wley3337 wley3337 commented Oct 19, 2022

Update amethyst.sample.yml to include all the defaults and the text description of what they do

Issue 1321

Copy link

@agoodshort agoodshort left a comment

Choose a reason for hiding this comment

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

Few comments/suggested changes to make this starter config file a bit more resilient

.amethyst.sample.yml Outdated Show resolved Hide resolved
.amethyst.sample.yml Outdated Show resolved Hide resolved
.amethyst.sample.yml Outdated Show resolved Hide resolved
.amethyst.sample.yml Outdated Show resolved Hide resolved
.amethyst.sample.yml Outdated Show resolved Hide resolved
.amethyst.sample.yml Outdated Show resolved Hide resolved
.amethyst.sample.yml Outdated Show resolved Hide resolved
.amethyst.sample.yml Outdated Show resolved Hide resolved
.amethyst.sample.yml Outdated Show resolved Hide resolved
Copy link

@agoodshort agoodshort left a comment

Choose a reason for hiding this comment

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

2 minor suggestion for better readability through an IDE

.amethyst.sample.yml Outdated Show resolved Hide resolved
.amethyst.sample.yml Outdated Show resolved Hide resolved
wley3337 and others added 11 commits December 8, 2022 20:59
Co-authored-by: Adrien Biencourt <[email protected]>
Co-authored-by: Adrien Biencourt <[email protected]>
Co-authored-by: Adrien Biencourt <[email protected]>
Co-authored-by: Adrien Biencourt <[email protected]>
Co-authored-by: Adrien Biencourt <[email protected]>
Co-authored-by: Adrien Biencourt <[email protected]>
Co-authored-by: Adrien Biencourt <[email protected]>
Co-authored-by: Adrien Biencourt <[email protected]>
Co-authored-by: Adrien Biencourt <[email protected]>
Co-authored-by: Adrien Biencourt <[email protected]>
Co-authored-by: Adrien Biencourt <[email protected]>
@wley3337
Copy link
Contributor Author

wley3337 commented Dec 9, 2022

@goodshort Sorry for the delay and thank you so much for the review! I tested out with my local and the " around the commands are definitely not needed ( outside the special characters ). Accepted all the suggestions. It's odd though there was only a single space on my local version for the comments rather than a tab space. I wonder why that looked like an indent 🤷

Let me know if you'd like me to squash and push back up :)

agoodshort
agoodshort previously approved these changes Dec 9, 2022
Copy link

@agoodshort agoodshort left a comment

Choose a reason for hiding this comment

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

Works fine on my machine. File is now a lot more readable and well documented. Thanks @wley3337

Copy link
Contributor

@cowboy-bebug cowboy-bebug left a comment

Choose a reason for hiding this comment

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

Some defaults such as select-*-layout are missing from default.amethyst 😃

@jabkoo
Copy link
Contributor

jabkoo commented Feb 3, 2023

+1 for @cowboy-bebug's suggestion. select-*-layout commands are not documented anywhere, so I would also like to see it mentioned in docs/configuration-files.md.

@wley3337
Copy link
Contributor Author

Hi all!

Been a bit swamped between work and life. Going to try and sort out the select-*-layout stuff in the next few days and get this updated

@Indyandie
Copy link

@wley3337 please note that this bug (#1419) could affect some of the configs

@wley3337
Copy link
Contributor Author

wley3337 commented Feb 19, 2023

Hi all!

Thank you all for your comments and patience. @Indyandie I'm not sure how to handle the possible conflicts. I added a block comment at the top to help with troubleshooting. Other than that, the only thing I can think of is to comment out the default values that are in default.amethyst and add the comment on each of those lines.

Something like:

# Commented out to avoid conflicts with `default.amethyst`
# only uncomment if you're going to change the default value below
# Move to the next layout in the list
# cycle-layout:
#   mod: mod1
#   key: space

@ianyh
Copy link
Owner

ianyh commented Mar 14, 2023

I might want to make some tweaks to this. Is it possible to give me permissions to push to this branch?

@vicmunoz
Copy link

vicmunoz commented Mar 25, 2023

Thanks!
Only missing:

window-max-count The max number of windows that may be visible on a screen at one time before additional windows are minimized. A value of 0 disables the feature. (default 0)
disable-screen-padding-on-inbuilt true to disable screen padding on in-built display (default false).

and a typo, is mod2 in default
swap-screen-cw:
mod: mod2
key: l

@prettymuchbryce
Copy link

We should merge this. Even if it's incomplete it is already way better than the current sample file, and this PR has been open for nearly 6 months.

@wley3337
Copy link
Contributor Author

wley3337 commented Apr 9, 2023

Added the missing two defaults above and fixed typo

@wley3337
Copy link
Contributor Author

wley3337 commented Apr 9, 2023

Oh, @ianyh I think I granted you access to push/pull let me know if that didn't work :) I pulled right before addressing the two comments with no diff so not sure if Github let you know :)

@ianyh ianyh merged commit fc20332 into ianyh:development Jun 3, 2023
@ianyh ianyh added this to the 0.21.0 milestone Jun 3, 2023
ickc added a commit to ickc/dotfiles that referenced this pull request May 21, 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.

8 participants