Lib/Indicator: Add enum for indicator position #640
Conversation
|
Builds fail because of this but it is not mine |
| * Boston, MA 02110-1301 USA. | ||
| */ | ||
|
|
||
| public enum Wingpanel.IndicatorPosition { |
There was a problem hiding this comment.
i am not sure where to put the enum definition where it doesnt get lost in code. Having its own file for three lines feels overkills.
Maybe putting this into Indicator.vala is fine? I agree with having its own file feels overkill.
| private void add_indicator (Indicator indicator) { | ||
| var indicator_entry = new IndicatorEntry (indicator, popover_manager); | ||
|
|
||
| switch (indicator.code_name) { |
There was a problem hiding this comment.
We have specific code for Applications menu and DateTime indicator elsewhere, did you revisit them and check that we don't need fix them?
user@elementary:~/work/tmp/wingpanel (master =)$ git grep -n APP_LAUNCHER
lib/Indicator.vala:21: public const string APP_LAUNCHER = "app-launcher";
src/Services/IndicatorSorter.vala:35: indicator_order[Indicator.APP_LAUNCHER] = 0;
src/Widgets/Panel.vala:143: case Indicator.APP_LAUNCHER:
src/Widgets/Panel.vala:197: case Indicator.APP_LAUNCHER:
src/Widgets/Panel.vala:251: if (indicator.code_name == Indicator.APP_LAUNCHER) {
user@elementary:~/work/tmp/wingpanel (master =)$ git grep -n DATETIME
lib/Indicator.vala:23: public const string DATETIME = "datetime";
src/Widgets/Panel.vala:158: case Indicator.DATETIME:
src/Widgets/Panel.vala:212: case Indicator.DATETIME:
src/Widgets/Panel.vala:255: if (indicator.code_name == Indicator.DATETIME) {
vapi/mutter-clutter.vapi:8363: DATETIME,
user@elementary:~/work/tmp/wingpanel (master =)$
| case Indicator.DATETIME: | ||
| case IndicatorPosition.CENTER: | ||
| indicator_entry.set_transition_type (Gtk.RevealerTransitionType.SLIDE_DOWN); | ||
| center_menubar.add (indicator_entry); |
There was a problem hiding this comment.
I'm not sure if we may leave this add() or should update to insert_sorted(). The previous code was add() because it's only DateTime indicator that is placed in the center. However, now we're allowing showing multiple indicators in the center, so I guess we need to revisit this.
I'm concerning that possibly the indicator order of CENTER could be different every time when the panel starts.
There was a problem hiding this comment.
Properly centering opens a lot of uneasy questions:
-Would it mean we do a IndicatorPosition.CENTER_LEFT and CENTER_RIGHT, and leave a pure center for the date?
-Would it mean we rewrite a whole lot of code for basically a fourth and fifth position ?
-Is it worth it?
Im aware i am doing a PR for a new functionality, but also it is directed at a niche hacky extension that relies on using an unsupported (for user software) packaging system
So when doing the code i was unsure whether to address this properly, or wait until there is actual demand for actually putting in the center in a more proper way. Or also only allow LEFT or RIGHT.
Tbh im fine with all. The easiest path out would be to remove CENTER position, id be fine with adding a CENTER_LEFT + CENTER_RIGHT but that would take time out of other eOS related project to implement.
Co-authored-by: Ryo Nakano <[email protected]>
Co-authored-by: Ryo Nakano <[email protected]>
Co-authored-by: Ryo Nakano <[email protected]>
2 participants
Fixes #608
Add a property for indicators for initial placement.
For Launcher and Datetime, an ugly workaround give them the correct positioning until the property is declared in their respective repos.
i am not sure where to put the enum definition where it doesnt get lost in code. Having its own file for three lines feels overkills.