Tray position like a module #2595
Conversation
There was a problem hiding this comment.
Wow that's really cool 😃
I appreciate you working on this. The tray has been a pain point for a long time and I have been dreading working on it forever.
This is surprisingly little code for what I thought would be a major undertaking.
Please let me know what your commitment for this PR is. Are you prepared to go through revisions until this is in a stable state? Or do you feel like this a "this is my code, take it or leave it" situation? Or something in between?
I don't want to push you until you lose interest, so it's good to know what kind of expectations you have here.
Of course feel free to at any point say that you no longer can/want to invest more time here.
We can just as well release this as an experimental, but mostly working, feature and improve it step by step.
While testing this, I found some issues, all of which happen using the following config:
[bar/example]
modules-left = text tray text
enable-ipc = true
tray-position = adaptive
width = 50%
offset-x = 50%
[module/text]
type = custom/text
content = A
[module/tray]
type = internal/tray- The
tray_manageruses the position received fromtray_pos_changeas the absolute position for the tray window. However, the dispatcher only sends the x position relative to the bar window. This is an issue if you have two monitors and place the bar on the right one or when the bar has someoffset-x. - The tray module does not receive the
tray_width_changesignal and thus never updates its width and doesn't produce a proper offset tag. This causes modules that are placed after the tray to appear immediately after the previous module (and the tray overlaps them)
I'm pretty sure that's because the module is not attached to the signal handler and thus doesn't receive any signals.
I think these two are the most important points to get the module into a experimental state that we can release to users.
There are also some conceptual things and edge cases to consider:
- If you set
tray-positionto anything butadaptivebut still have a tray module, the tray module will display the tray, but the bar will still cut off parts of the bar inbar::parse. We should enforce that iftray-position = adaptive, there must be exactly one tray module. And if it is anything else, no tray module must exist. We could also ditchtray-position = adaptiveand just check for the existence of a tray module. - If there is too much text on the bar, any excess text will just drop off the bar on the right. We have to decide (and properly handle) what happens when tray is shoved off the right side of the bar like that. My idea is that in that case the tray just stays at the right edge of the bar and never disappears. This can also easily be enforced by the
tray_managerwhen it handles thetray_pos_changesignal. - We also have to think about how we want to move all the tray configurations that are currently into the bar section into the tray module. But this doesn't have priority at all yet, we can easily tackle this once the tray is stable.
These points are not yet important if we just release the tray module as an experimental feature, but we will need to address them to get to a stable state.
define how the tray positioning will be handled in the config. Will tray-position be deprecated?
In order to not break existing configs, we need to still support tray-position. Depending on whether we merge this as an experimental feature or try to get a stable implementation in this PR, we can already tell people to switch and that all the tray settings in the bar section will be deprecated. But leave it as-is for now.
figure out how to get the position where a tag will be placed in dispatch.cpp. The current approach only works on the left and in the center
I think the solution here is to not immediately move the tray but only emit the tray position signal once the rendering is finished. Either the dispatch or the renderer need to keep track in which alignment and which relative position in that alignment the tray has to appear and at the end of the render cycle calculate the actual position.
|
Hi, thanks for your quick and detailed response!
I'd ultimately like to see this become stable and I'm committed to continue working on it. Since I don't really understand some parts of the code works(modules, formatter etc.) I'll need some help in these areas and definitely appreciate any suggestions. The next few months will be quite busy for me so it may be that I'll need to take a break for a while but I'll definitely let you know before.
I also noticed this error and the tray positioning has been the most annoying issue I faced. Moving the tray position signal to the renderer sounds like a good idea. I never liked it in the dispatch. double absolute_x = m_rect.x + block_x(alignment);
in dispatch: double current_relative_x = renderer.get_x(*m_ctxt);For now I'll look into the problem with the tray_width_change signal and incorporate your suggestions. |
Just let me know if anything is unclear. Our code documentation isn't that good and a lot of code (especially the base module code) has been here for a long time (even before I started working on polybar) and its behavior isn't always obvious for me either.
That's completely fine :) The tray positioning is terrible. It's the main reason why I got stuck in my last big tray PR #1615. Unfortunately, neither of your proposed solutions work, both of them are still relative to the bar window. I think the following could work: Once the dispatcher is finished, it calls into the renderer and the renderer can calculate the position relative to the inner area of the bar like this: double absolute_x = block_x(ctxt.tray.first) + ctxt.tray.second;It then sends this to the tray_manager and the tray manager is responsible for calculating the absolute position. The tray gets passed the m_opts.orig_x = m_bar_opts.inner_area(true).x + evt.cast(); |
EDIT: fixed tests |
79415fa to
19347eb
Compare
There was a problem hiding this comment.
This is coming along nicely :)
I have not had any problems when testing it out. The reviews are mainly cosmetic/architectural ones.
Please also run clang-format over all files that you have changed so that we have consistent formatting (Style Guide).
You can run clang-format for all changed files using this command:
git diff --pretty="" --name-only "$(git merge-base HEAD upstream/master)..HEAD" | xargs clang-format -i --style=file
Note that this will completely mess up non-C++ files (cmake etc.), so just don't commit those ;)
After this round of reviews this should be in a state where we can merge this as an experimental feature. After that, we should think about the three optional points in my first review.
What do you prefer? Should we merge this as an experimental feature or first bring it into a stable state in this PR?
|
This may also fix #651, but I haven't tested it yet. |
|
Hi, |
|
Thanks for letting me know :) There is no need to rush here, take your time |
|
Hi Polygon Team, I'm new to the polygon/linux and github world. How i can "download" / "test" this tray-modul? |
|
@Luekuu This is very much not ready to use for end users. If you really want to, you can clone @raffael0's fork and compile the |
348e51d to
4e47bc0
Compare
|
Will review in depth later. Your latest commit messed up the formatting in a bunch of files. EDIT: See also our style guide: https://polybar.readthedocs.io/en/latest/dev/style-guide.html |
|
Sorry about that. I have no idea how that happened. Should be fixed now. |
Maybe your IDE auto-formatted the files. |
There was a problem hiding this comment.
Alright, looks good now 😃
Things to do now to merge this as an experimental feature:
- Add a changelog entry: https://github.com/polybar/polybar/blob/master/CONTRIBUTING.md#changelog
- Addressing the remaining reviews (mainly cosmetic)
- Remove the
bar_settingsargument fromtray_manager::setup - Resolve merge conflicts. Let me know if you need help with that.
After merging, we need to take these steps to make this feature stable
- Automatically hide the tray window when no tray position is set in the renderer
- Consider removing
tray-position = adaptiveagain and just activate the tray when a tray module exists. The existence of a tray module should override any value fortray-position - Fix the tray window being pushed off the bar. The tray window should always be fully within the bar window¨.
- Also support applicable tray settings in the tray module (
tray-maxsize,tray-background,tray-foreground,tray-padding,tray-scale, and maybetray-offset-y) - Emit deprecation warnings for
tray-positionand all other tray settings in the bar section.$
These can be implemented step-by-step in separate PRs. I'll create an issue for them once we merge this.
b5354ff to
a2b4553
Compare
|
Hi. these commits should address all your points:
|
Codecov Report
@@ Coverage Diff @@
## master #2595 +/- ##
==========================================
- Coverage 13.62% 13.58% -0.05%
==========================================
Files 149 152 +3
Lines 11286 11339 +53
==========================================
+ Hits 1538 1540 +2
- Misses 9748 9799 +51
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
Is this ready to be merged or am I missing an open issue? |
|
Great! |
|
@raffael0, hello! I am trying to use this. However, there does not appear to be any documentation for it. I have tried adding it to my bar and using |
|
Hi, |
What type of PR is this? (check all applicable)
Description
This PR fixes the long-standing issue of only having 3 positioning options for the system tray. It does this by adding a new module which acts as placeholder. The actual tray window is then placed onto that tray module. Resizing of the tray and repositioning of the module is handled via signals.
These changes should also make it easier to completely break the tray handling out of the main bar.cpp into the new tray module.
Related Issues & Documents
Fixes: #1526
Fixes: #314
Documentation (check all applicable)
Since this PR adds a complete new option for tray positioning this has to be reflected in the wiki
TODO / Issues
I'd love some input on these issues