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

Refactor Panel, Layout, Expo and Scale to handle various panel configurations #1003

Closed
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@autarkper
Contributor

autarkper commented Aug 6, 2012

Expo and Scale don't work well with the non-default panel configurations ("classic" and "flipped") and auto-hide, and there are also some other problems when using non-standard panel sizes. This pull request is a large step towards making configurable panels work in a predictable fashion.

See also: #951.

@Cobinja

This comment has been minimized.

Member

Cobinja commented Aug 6, 2012

@autarkper: could you take another look at this? currently it breaks setting bottom panel height without cinnamon restart. the position is not adjusted (probably to not calling LayoutManager.updateBoxes)

@autarkper

This comment has been minimized.

Contributor

autarkper commented Aug 6, 2012

@Cobinja: The way I see it, it is already broken in master. It is one of the remaining warts, but I don't think I have made it any worse than it already is. Please check again and correct me if I'm wrong.

@Cobinja

This comment has been minimized.

Member

Cobinja commented Aug 6, 2012

@autarkper: I checked. In master it works fine.
Problem seems to be the deletion of lines 784 and 808 from panel.js

@autarkper

This comment has been minimized.

Contributor

autarkper commented Aug 6, 2012

@Cobinja: Well then I'm doubly stumped, once for not being able to reproduce correct behavior in current master, twice for not being able to fix it on my branch. Re lines 784 and 808, they have not been deleted, but reworked. The call of Main.layoutManager._updateBoxes is done in layout.js instead; duplicating the call in panel.js does not have any perceivable effect.

@Cobinja

This comment has been minimized.

Member

Cobinja commented Aug 6, 2012

@autarkper: you need to call layoutManager._updateBoxes every time the panel height is changed. otherwise the y-position of the bottom panel is not adjusted.

@autarkper

This comment has been minimized.

Contributor

autarkper commented Aug 6, 2012

If you put a log statement in _updateBoxes you will see that it is called. Please note that the code works for the top panel.

@Cobinja

This comment has been minimized.

Member

Cobinja commented Aug 6, 2012

@autarkper: I looked again. Your 3rd commit solves it.
Imho it would be better to just call _updateBoxes from panel.js instead of doing all that autohide change stuff in layout.js additionally (inc. avoiding the "interesting" naming scheme with onPanelAutoHideChanged reacting to changes in "panel-top-height" and "panel-bottom-height").

@autarkper

This comment has been minimized.

Contributor

autarkper commented Aug 6, 2012

The latest commit solves the problem with the desktop not resizing correctly after changing the size of the bottom panel.

@Cobinja: There must be something different in our configurations, because I had to do even more to make it work around here. As I've said before, simply calling _updateBoxes from panel.js does not work for me. I agree that the names are not very descriptive anymore; I'll try and find something more suitable.

@Cobinja

This comment has been minimized.

Member

Cobinja commented Aug 7, 2012

@autarkper: The problem with the desktop size is solved. Still, there's something that's not really a problem. Showing and instantly hiding a panel when changing the height with autohide on looks somehow weird (at least to me).

@autarkper

This comment has been minimized.

Contributor

autarkper commented Aug 7, 2012

@Cobinja: Thanks for testing! Flashing the panel bugs me too, a bit, but given that users probably won't be changing panel settings very often I hope it'll be acceptable. Actually, I think the ability to resize panels currently isn't very useful anyway, since the contents doesn't scale.

@autarkper

This comment has been minimized.

Contributor

autarkper commented Aug 7, 2012

I have squashed the commits somewhat.

@Cobinja

This comment has been minimized.

Member

Cobinja commented Aug 7, 2012

@autarkper: For "autohide on" i think we don't have to adjust the desktop size, since it's not affected by autohidden panels.

@autarkper

This comment has been minimized.

Contributor

autarkper commented Aug 7, 2012

@Cobinja: Is that really so? Turning auto-hide on should reclaim the space previously occupied by the panels.

@Cobinja

This comment has been minimized.

Member

Cobinja commented Aug 7, 2012

@autarkper: Sry, that wasn't expressed well. Better: While autohide is turned on, I think we don't have to adjust the desktop size when setting a new panel height, since it's not affected by an autohidden panel, and thus we can maybe avoid showing and instantly hiding the panel.

@autarkper

This comment has been minimized.

Contributor

autarkper commented Aug 7, 2012

@Cobinja: OK, that makes sense. I'll check.

@autarkper

This comment has been minimized.

Contributor

autarkper commented Aug 7, 2012

@Cobinja: I have investigated making an exception when auto-hide is on and not changing, but the result is not satisfactory. I won't have much more time to work on this issue, so I'll have to leave it as it is.

@autarkper

This comment has been minimized.

Contributor

autarkper commented Sep 5, 2012

This will no longer merge cleanly to master; there is currently a conflict in panel.js that cannot be mechanically resolved. Unfortunately I'm not able to work that out for the time being.

@autarkper

This comment has been minimized.

Contributor

autarkper commented Sep 5, 2012

I'm closing this for now, as this needs more testing. I would also like to investigate whether we can support multi-monitor configurations better than today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment