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 tree width options #1018

Merged
merged 3 commits into from
Dec 13, 2020
Merged

Add tree width options #1018

merged 3 commits into from
Dec 13, 2020

Conversation

pgaskin
Copy link
Collaborator

@pgaskin pgaskin commented Dec 12, 2020

closes #990
closes #772

@pgaskin
Copy link
Collaborator Author

pgaskin commented Dec 12, 2020

A few notes:

  • I'm not completely familiar with the UI code, so I'm not 100% sure this doesn't cause issues elsewhere, but it seems to work fine right now.
  • Since I'm using an int between 1 and 100, we lose precision (it used to divide by 3), but this is mostly irrelevant since terminal windows aren't wide enough to make this have a significant effect. If this is an issue, I could parse it as a double or a fraction with a numerator/denominator instead.
  • At high percentages (e.g. 99%) with small windows, the text on the right pane may spill over to the next line since the headers don't shrink. If this is an issue, I could set a minimum width for the right pane, but it isn't really usable when it's that small in the first place.

Copy link
Collaborator Author

@pgaskin pgaskin left a comment

Choose a reason for hiding this comment

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

Tested tree/library/playlist views, mouse events, resizing window, setting window size from rc/autosave/command-line/remote.

@nefthy
Copy link
Collaborator

nefthy commented Dec 13, 2020

  • I'm not completely familiar with the UI code, so I'm not 100% sure this doesn't cause issues elsewhere, but it seems to work fine right now.

IMO it's better to handle the width in columns since that is the basic underlying unit.'

@pgaskin
Copy link
Collaborator Author

pgaskin commented Dec 13, 2020

What do you think of renaming it to tree_width and making it accept either a fixed value (later constrained to the max width of the window) or a percentage (if there's a percent sign at the end)?

@nefthy
Copy link
Collaborator

nefthy commented Dec 13, 2020

sounds good

@pgaskin
Copy link
Collaborator Author

pgaskin commented Dec 13, 2020

Actually, I think I have a better idea: adding an optional tree_width_max option to limit the maximum width, and increasing the maximum tree_width_percent to 100%. This will allow both: setting a fixed size and still making it work properly at smaller window sizes, limiting the size only on large window sizes, and keeping the old behaviour.

@nefthy
Copy link
Collaborator

nefthy commented Dec 13, 2020

Also a good option.

@pgaskin pgaskin changed the title Add tree_width_percent option Add tree width options Dec 13, 2020
Copy link
Collaborator Author

@pgaskin pgaskin left a comment

Choose a reason for hiding this comment

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

Tested tree/library/playlist views, mouse events, resizing window, setting window size from rc/autosave/command-line/remote with:

  • tree_width_percent=100 tree_width_max=60
  • tree_width_percent=33 tree_width_max=60
  • tree_width_percent=33 tree_width_max=0
  • tree_width_percent=1 tree_width_max=9999

@pgaskin
Copy link
Collaborator Author

pgaskin commented Dec 13, 2020

@nefthy, this is ready for review now.

@pgaskin
Copy link
Collaborator Author

pgaskin commented Dec 13, 2020

I took a closer look at the code, and I'm now fairly certain this won't cause issues elsewhere.

Copy link
Collaborator

@nefthy nefthy left a comment

Choose a reason for hiding this comment

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

Looks good, but please rebase, as there is a conflict with another merge

@pgaskin pgaskin requested a review from nefthy December 13, 2020 19:56
@nefthy
Copy link
Collaborator

nefthy commented Dec 13, 2020

thanks

@flyingmutant flyingmutant merged commit 0e8632d into cmus:master Dec 13, 2020
@pgaskin pgaskin deleted the tree-width-option branch January 12, 2021 18:42
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.

How to change treeview width Is there a way to make playlist list smaller?
3 participants