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

feat: Add orientation support for clock, button, label, and sys_info #530

Merged
merged 5 commits into from
Apr 6, 2024

Conversation

ClaireNeveu
Copy link
Contributor

After making the switch to hyprland ironbar is by far the best dock solution I've found. Most wayland dockbars that aren't built into a big DE have very poor support for vertical docks or lack a proper program list/launcher. The one big issue I've run into is that the clock is incorrectly rotated when using a vertical bar. In this discussion you mentioned you wanted to keep the current behavior as an option.

I went ahead and added orientation support for clock as well as label and button which some people were asking for. I also created an enum for orientation and moved deserialization into serde and out of an ad-hoc function.

I also added a test config where you can see that all variations (default, horizontal, and vertical along with shorthand) are functioning properly.

Please let me know if there's anything you need on my end to get this merged, I didn't see any contribution guidelines or standards in the repo.

@ClaireNeveu
Copy link
Contributor Author

Just noticed there are a few module's I'm not using that currently still rotate based on bar position (upower, music, sysinfo) so I'd be happy to add orientation support for those as well if you like.

Copy link
Owner

@JakeStanger JakeStanger left a comment

Choose a reason for hiding this comment

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

Hey, thanks for the PR. I really appreciate the work you've put in here. Vertical bars have needed some love for a very long time so I'm very glad to finally see them get some.

The code generally lgtm, there's a few picky points I've raised but nothing of actual concern, other than the build failures.

Can you also ensure your commits follow the convention commits standard please, so that they get included in the changelog (and you get credited)

src/modules/clock.rs Outdated Show resolved Hide resolved
src/config/common.rs Outdated Show resolved Hide resolved
src/modules/custom/label.rs Outdated Show resolved Hide resolved
test-configs/orientation.yaml Outdated Show resolved Hide resolved
@JakeStanger
Copy link
Owner

Just noticed there are a few module's I'm not using that currently still rotate based on bar position (upower, music, sysinfo) so I'd be happy to add orientation support for those as well if you like.

If you want to take these on too, I'd really appreciate it as well. No pressure if you're not up for it though :)

@ClaireNeveu ClaireNeveu changed the title Add orientation support for clock, button, and label feat: Add orientation support for clock, button, and label Apr 5, 2024
@ClaireNeveu ClaireNeveu force-pushed the master branch 2 times, most recently from 0b245d0 to 6aa7455 Compare April 5, 2024 17:32
@ClaireNeveu
Copy link
Contributor Author

ClaireNeveu commented Apr 5, 2024

I addressed the PR feedback and I added orientation support for sysinfo. I took a stab at upower and music but I got hung up on rotating the icons (if you know how to do this I'd be happy to do the rest). Notably right now upower at least doesn't really have proper behavior on vertical bars since only the label changes when the whole button needs to change.

IMO it'd be better to just remove line 184 there, you'd get a more functional widget in vertical mode.

label.set_angle(info.bar_position.get_angle());

@ClaireNeveu ClaireNeveu changed the title feat: Add orientation support for clock, button, and label feat: Add orientation support for clock, button, label, and sys_info Apr 5, 2024
Copy link
Owner

@JakeStanger JakeStanger left a comment

Choose a reason for hiding this comment

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

A few more small changes please, and then if you can squash your commits down to remove the PR-related noise I think we're getting there.

I took a stab at upower and music but I got hung up on rotating the icons (if you know how to do this I'd be happy to do the rest).

Sure, looking at re-doing the icon code is actually next on my agenda as part of #511 anyway so I can pick that up.

Feel free to remove that line from UPower in the meantime.

src/config/common.rs Outdated Show resolved Hide resolved
src/modules/custom/box.rs Outdated Show resolved Hide resolved
src/modules/sysinfo.rs Show resolved Hide resolved
test-configs/orientation.yaml Outdated Show resolved Hide resolved
@ClaireNeveu
Copy link
Contributor Author

ClaireNeveu commented Apr 6, 2024

@JakeStanger Resolved all your feedback. Is there a particular reason you want me to squash my commits manually instead of using Github's squash merge feature?

@JakeStanger
Copy link
Owner

Is there a particular reason you want me to squash my commits manually instead of using Github's squash merge feature?

GH's squash merge will reduce everything into a single commit, I just want to filter out the PR-related commits :) There's still several changes this PR makes so I'd like to keep them separated so it's clear in the changelog:

  • Introduction of orientation option
  • Change of UPower
  • Introduction of test file
  • Anything else I've missed

@ClaireNeveu
Copy link
Contributor Author

Gotcha, I'll try to get it down to relevant commits. It does mean that the commits themselves won't be strictly what is mentioned in their commit messages since interactive rebase can only squash into adjacent commits.

@JakeStanger
Copy link
Owner

interactive rebase can only squash into adjacent commits.

You can actually squash into any commit! You can re-order the lines in the editor to change what they're adjacent to, and it should work it out. If you want a hand with that let me know.

Copy link
Owner

@JakeStanger JakeStanger left a comment

Choose a reason for hiding this comment

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

Code all LGTM - if you can just fix formatting too pls, and then we're there. Cheers for your hard work on this

@ClaireNeveu
Copy link
Contributor Author

I don't have a separate commit for actually introducing the test config but I'll preserve the commit where I converted it to corn.

@ClaireNeveu
Copy link
Contributor Author

Ok, I reworked my commits. I've gotten used to squash merges but in the future I'll try to maintain better commit hygiene on PRs for ironbar.

@JakeStanger
Copy link
Owner

I've gotten used to squash merges but in the future I'll try to maintain better commit hygiene on PRs for ironbar.

I appreciate everyone works differently and I know it's quite an involved way of working compared to a lot of repos, so appreciate the extra effort.

It's mostly me being lazy so I don't have to write the changelog myself, but I do think it helps keep commits tidy and sensibly sized, which is so very useful when I need to go back through the history.

All boxes ticked, commits look good, code all LGTM now so will merge. Cheers again :)

@JakeStanger JakeStanger merged commit 9a71c69 into JakeStanger:master Apr 6, 2024
8 checks passed
@ClaireNeveu
Copy link
Contributor Author

@JakeStanger Thanks for all the help and the quick merge!

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.

2 participants