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

i3: Default workspace & Workspace output assignment #1967

Closed
wants to merge 1 commit into from
Closed

i3: Default workspace & Workspace output assignment #1967

wants to merge 1 commit into from

Conversation

SebTM
Copy link
Collaborator

@SebTM SebTM commented Apr 29, 2021

Description

Hello,

I'm a new to NixOS (and also nix, home-manager and the whole ecosystem). This is my first contribution after I tried to build my personal I3-config with home-manager and found some cases I was not able to do.

If there is a need I'm happy to adjust, please describe/comment in the code :-)
I tried my best to follow the contribution-guidelines and the checklist - if I missed something please inform me.

Cheers!

Possibly related to #695 but does not solve it completely.

Checklist

  • Change is backwards compatible.

  • Code formatted with ./format.

  • Code tested through nix-shell --pure tests -A run.all.

  • Test cases updated/added. See example.

  • Commit messages are formatted like

    {component}: {description}
    
    {long description}
    

@SebTM SebTM changed the title WIP: I3-Config improvements I3-Config improvements Apr 29, 2021
@SebTM SebTM changed the title I3-Config improvements services.xsession.windowManager.i3: Extend configuration Apr 29, 2021
@sumnerevans
Copy link
Member

Could you make this more generic so that it can be used in sway as well? I think that would involve moving more of the functions and the options to the lib folder.

@SebTM SebTM requested a review from sumnerevans as a code owner May 1, 2021 16:08
Copy link
Member

@sumnerevans sumnerevans left a comment

Choose a reason for hiding this comment

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

I think it would be good to split this into two PRs:

  • add defaultWorkspace option.
  • add workspaceOutputAssign option.

I agree with @rycee that it doesn't make much sense to have the variables option since that should be handled by Nix variables.

@rycee
Copy link
Member

rycee commented May 5, 2021

Please mind the commit message length, the subject line should at most 60 characters, ideally 50. At the moment it seems to be 172 characters.

@SebTM
Copy link
Collaborator Author

SebTM commented May 8, 2021

Thanks for your Feedback!

  • I moved the options to the lib-files
  • I removed the variables option as it was really not needed, a "miss-thinking" in the first try I guess :)
  • I adjusted the description to something more understandable
  • I adjusted the commit message

I don't see the advantage of introducing a sortOrder to keybindings as there is no real impact on the rest of the orders as far as I found out? (and more complexity?)

If you give me feedback to just split up the PR, I'm happy to do this, until everything is ok it's easier for me just to keep up2date one :)

@SebTM SebTM changed the title services.xsession.windowManager.i3: Extend configuration i3: Extend configuration May 9, 2021
@SebTM SebTM changed the title i3: Extend configuration i3: Default workspace & Workspace output assignment May 9, 2021
Introduce new options in config, defaultWorkspace and workspaceOutputAssign.
@sumnerevans
Copy link
Member

Sorry for the delay getting back to this. I think it looks pretty good now. Let's go ahead and split it up into two PRs so that they are a bit easier to handle.

@SebTM
Copy link
Collaborator Author

SebTM commented May 13, 2021

No Problem, spitted into two PRs mentioned this :-)

@SebTM SebTM closed this May 13, 2021
@SebTM SebTM deleted the improvements_i3_var_outputs branch October 9, 2021 14:52
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.

3 participants