Closed thesquash closed 2 years ago
I think this function is very good. It can set the position of panel applet from three relative positions: start, center and end.
Also, has anybody actually tried applying this patch / PR and using the patched version of the panel yet? Because if not, I wish somebody would. Discussing code is one thing; testing it is another.
Thanks in advance, as always. I appreciate the help.
Note that I am running a very customized panel configuration, breaking this sort of thing is exactly what we have to worry about here
No, I'm simply an idiot who uses link-time optimization, so I was able to get away with defining a (semi-public) function with the same name as another (public) function. It took me all day to figure this out, though, so I think I got punishment enough for the mistake.
The problem lay within mate-panel/applet.c
. On line 1365, I defined a function called mate_panel_applet_get_size ()
, which accepted an AppletInfo *
and returned the number of "cells" an applet used on the panel. I chose the name because similar "helper" functions in that file were named similarly, and like the other helper functions (e.g. mate_panel_applet_get_position ()
), this function could not be made private / static
because it was used in other source files, including mate-panel/panel-toplevel.c
.
Unfortunately, the name mate_panel_applet_get_size ()
was already used by a function in libmate-panel-applet
. That function does pretty much the same thing, except that it takes a MatePanelApplet *
parameter as input instead of an AppletInfo *
. When the dynamic loader resolves all the symbols used by all the libraries, the panel executable itself, and the applets, it confused the two functions, and one got loaded under all circumstances, even when the other should have been called.
I did not notice this at first because I often compile applications with link-time optimization for code size reasons. The link-time optimizer in GCC is smart enough to realize that the two functions are not the same, and my version of the function is "privatized" (made non-global and thus invisible to the dynamic loader). Thus when compiled with LTO, my bug does not show up. But for everybody else with some sanity left, they see a crashing panel as the wrong function with the wrong input is called.
TL;DR: It's fixed now, and I've put a sticky note on my monitor: "Do NOT use LTO when testing new pull requests!" :-)
To prevent the unexpected panel applet movement that @lukefromdc profiled above, I added the (deprecated) panel-right-stick
property back into the code, but implemented the backward-compatibility correctly this time.
I also updated the "Ubuntu" panel layout very slightly: One panel applet (the shutdown icon, I think) was supposed to be positioned at the far right of the top panel. It was positioned correctly when the panel positioned the left side of applets relative to the right side of the panel (panel-right-stick
), but is positioned with a gap between the right of the applet and the right of the panel when my center-stick patch is applied. (My center-stick patch also changes the semantics of right-stuck applets slightly; now the right side of the applet is positioned relative to the right side of the panel, which makes more sense than the former behavior, at least IMHO.)
I hope this latest change does the trick!
I'm puzzled. The Debian test build passed, but the Fedora one didn't. Still, it appears both containers successfully compiled my changes.
Is this another instance of the build testing system being broken?
Just retested, still got moving applets some of the time but not all of the time. Space issues may have triggered it in my second test, in which the clock (3ed from right) disappeared. Also got some moving applets on rollback this time around too, but this would be extremelty uncommon for normal users as it would require a major version rollback. Again, not a blocker to me so long as users are aware of this, remember we are on 1.27 so just not cherrypicking would mean users only see this if they update a distro to a new version, compile MATE locally and move up a major version, or get a major version update on a rolling release
@thesquash Why did you add the right-stick
code
still got moving applets some of the time but not all of the time. Space issues may have triggered it in my second test, in which the clock (3ed from right) disappeared.
It's unacceptable for this PR to move applets unexpectedly, so this can't be ignored. However, any chance of showing me a screenshot of your panel layout under normal circumstances, and one of the panel after loading my patch, and one when you try to move stuff around but applets disappear? Because I'm working totally blindly right now -- if I could replicate your panel layout (or at least come somewhere close to it) then I could probably fix all these bugs. As it is, though, I can't reproduce what you're talking about.
Why did you add the
right-stick
code
I thought I already explained this above, but let's say I didn't: For backward compatibility (or actually, at least forward compatibility). In the first revision of the PR, I included code to make applets end-relative if the right-stick property was set on the applet, but the code did not quite work as intended. Then I removed the old right-stick code entirely, and @lukefromdc said his right-stuck panel applets jumped over to the left unexpectedly. So now I've added the right-stick compatibility code back in, albeit in a more functional form, so that people's applets won't jump to the other side of the panel; at most they'll now move a few pixels, since the right side of an end-relative applet is positioned relative to the end of the panel, whereas right-sticking positioned the left side of an applet relative to the right side of the panel!
So, in other words, maybe eventually we can remove the right-stick property, but IMHO it should be kept around for a stable release or two at least. Do users really read release notes anyway? No. Distributors read release notes. And distributors almost never have been known to, say, display a dialog box upon system upgrade that says "your panel applets will be rearranged upon the next login, be prepared for that to happen!" So really, it's our job to implement some level of backward compatibility so we don't get a flood of irate users come along and create issue after issue describing this "bug".
I said the preceding text as both a programmer and a user myself, and I think most users would agree.
I am on the roar now, might be 36 hours or so.before I can send a panel screenshot but will do so. It's a single panel on the bottom, last tjree applets left to right are clock, trash, and logout, usually with no extra space between them.
Overall on that box is menubutton, show desktop, a bunch of app launchers, window list on the left. aftet space set aside for thr windoe list ai have netspeed, inhibit, force-quit (order of those two sometimes reversed when moved by accident), workspace switcher, hardware monitor, cpufreq, tray, volume control applet,clock, trash, and logout.
On 5/13/2022 at 3:20 PM, "Gordon N. Squash" @.***> wrote:
still got moving applets some of the time but not all of the time. Space issues may have triggered it in my second test, in which the clock (3ed from right) disappeared.
It's unacceptable for this PR to move applets unexpectedly, so this can't be ignored. However, any chance of showing me a screenshot of your panel layout under normal circumstances, and one of the panel after loading my patch, and one when you try to move stuff around but applets disappear? Because I'm working totally blindly right now -- if I could replicate your panel layout (or at least come somewhere close to it) then I could probably fix all these bugs. As it is, though, I can't reproduce what you're talking about.
Why did you add the
right-stick
codeI thought I already explained this above, but let's say I didn't:
For backward compatibility (or actually, at least forward compatibility). In the first revision of the PR, I included code to make applets end-relative if the right-stick property was set on the applet, but the code did not quite work as intended. Then I removed the old right-stick code entirely, and @lukefromdc said his right-stuck panel applets jumped over to the left unexpectedly. So now I've added the right-stick compatibility code back in, albeit in a more functional form, so that people's applets won't jump to the other side of the panel; at most they'll now move a few pixels, since the right side of an end-relative applet is positioned relative to the end of the panel, whereas right-sticking positioned the left side of an applet relative to the right side of the panel!So, in other words, maybe eventually we can remove the right-stick property, but IMHO it should be kept around for a stable release or two at least. Do users really read release notes anyway? No.
Distributors read release notes. And distributors almost never have been known to, say, display a dialog box upon system upgrade that says "your panel applets will be rearranged upon the next login, be prepared for that to happen!" So really, it's our job to implement some level of backward compatibility so we don't get a flood of irate users come along and create issue after issue describing this "bug".I said the preceding text as both a programmer and a user myself, and I think most users would agree.
-- Reply to this email directly or view it on GitHub: https://github.com/mate-desktop/mate-panel/pull/1310#issuecomment- 1126370627 You are receiving this because you were mentioned.
Message ID: <mate-desktop/mate- @.***>
Here is a scaled-down screenshot of my panel: This is the desktop not the laptop I tested on the other night, but only difference is absence of the netspeed applet as I have conky working well on the desktop with that info
I couldn't replicate exactly the behavior you encountered with applets disappearing, but I did notice the moving applets, and I fixed that and some other bugs that I should have noticed earlier. The only "disappearing" applet I noticed was the notification area applet, which randomly appeared as a 1 pixel wide line on the panel if no system tray icons were loaded; focusing the panel with Ctrl-Alt-Tab and then Tab-ing around to the notification area resized the notification area and shows the handle. Everything else works as intended, as far as I can tell (which may not be worth much!). I hope I fixed the wonkiness this time.
Forget what I wrote previously here regarding the applet dump. I figured out what's wrong. (And replicated your findings, finally.)
You're witnessing the results of applets getting rearranged because the way applets are positioned post-application of this patch is different from how they are positioned normally. (I'll say it again: The left side of applets, even right-stuck applets, were positioned relative to one end of the panel [depending on whether right-sticking was enabled]; my patch positions the right side of right-stuck applets relative to the right of the panel, the center of centered applets relative to the center of the panel, and the left side of everything else relative to the left side of the panel.)
Sorry but there's nothing I think I can do about it. I really think my method is more logical, and doing otherwise would be an injustice to users -- unless someone else can think of a way around the problem.
Thinking about this myself, I can think of three ways to mitigate this, the first two involving a flag in GSettings. The first would involve a new boolean variable assigned to each applet; the default setting for this variable is FALSE
. If this variable, shall we call it updated-position-setting
, is FALSE
(as it would be right after upgrading the panel to the version with my changes), and the applet is right-stuck, the MATE Panel treats the position
setting as it did in prior versions of the panel, that is to say that the left side of the object is positioned relative to the right-side of the panel. This flag is promptly unset by the MATE Panel code, and the position
setting updated to account for the object's width, thus upgrading the user's configuration automatically.
The second approach is to do as above, but distinguish old-style positions from new-style positions by the sign of the position
setting; a negative one might indicate new-style positions. This has an advantage in that it doesn't require a new setting, but has the disadvantage that it looks stupid (and makes the code look stupid too).
The third approach is to test the panel-right-stick
property that we already have, and if it's set to TRUE
, then adjust for the positioning semantics, then set the new position
setting and set panel-right-stick
to FALSE
. This is probably the best option since it uses a setting / property that we already have.
All of the above have the disadvantage that if the user ever downgrades their panel, the panel layout will look like cruft then. However, if the user ever manually repositions any applets after upgrading and then does a downgrade, they'd have a screwed-up panel anyway, no matter what we do.
In that case, barring any further comments, I think I'll go with option (3).
That will be seveeal days away, as I am about to depart on a 1,000 mile road trip
I manually position applets and have never intentiomally tried to invoke a "right stick" though I do lock applets to the panel. So far, every change to this has only had a minor effect on this behavior.
Had this w both machines, but I normally copy hidden/config files along with OS installs from machine to machine
I will not block this or any other PR because I am.often on the road (due to not having reliable housing) and thus cannot guarantee availability for retests at.any given time. Not like things were for me back in 2016.
All right -- apply this patch, and nobody should have unexpectedly-moving panel applets anymore. And if you don't move any applets, downgrading should work too. But once you position some applets anew with this patch applied, do not downgrade, because the new applets will wind up in the wrong places. Still, I think, short of finding any real bugs now, that this patch has as good behavior as any.
Please review this round of patches (when time permits).
Wow! I might have actually done something right -- good to hear!
OK, we now need to know if these changes satisfy @vkareh and we need another review
Gosh -- I know I actually found (and fixed) those things at one point. I guess I never committed my changes though. Great catches, though -- I'm highly appreciative for your consistently good eye for programming mistakes, even ones that don't cause unexpected behavior. Thumbs up for helping a screw-up like me!
Just retested, no icons moved. Looks good to go on this end. Do we have other problems still outstanding or is this good to go?
Can we merge? Something with docker build and fedora is bugging travis... but seems to be the case in all PRs
Pipelines are fixed now. Merging
Conventionally, the applets on a MATE Panel are positioned relative to the left edge of the panel (if the panel is horizontal) or the top edge (if the panel is vertical). There has also been some (buggy) support for positioning of applets relative to the right (or bottom) edge of the panel, so that applets on the right side of the panel will stay on the right side even if the user changes screen resolutions or if the panel changes size for some other reason.
However, many users want to also place applets at or near the center of their panel(s). There is no such conventional support for positioning applets relative to the center of the panel, so users have positioned applets near the center of the panel -- but the position recorded is relative to the left side of the panel. As such, the applets will almost certainly shift over to the left or right slightly if the panel is ever resized, and the user will have to reposition all those centered applets yet again. This is especially frustrating if the user switches monitors on a regular basis!
This patch radically revamps the MATE Panel's positioning framework, and deprecates the original "right-stick" feature. To replace the right-stick feature, this patch instead associates an "edge relativity" setting with each and every panel applet: An applet can be relative to the start (left/ top), end (right/bottom), or center of the panel. This setting can be changed using DConf/GSettings, using a custom panel layout file, or even by simply dragging the applet to the appropriate place on the panel. (Conventionally, applets are not even right-stuck automatically even when the user drags the applet over to the far right of the panel!) As a bonus, when the user drags an applet across the center of the panel, the applet will temporarily "stick" to the very center of the panel, to allow the user to very precisely align any applet they wish.
Fixes: #426