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

feature: Remove unneeded padding #219

Merged
merged 2 commits into from
May 18, 2020
Merged

Conversation

Finii
Copy link
Contributor

@Finii Finii commented Mar 26, 2020

[why]
The spacing between icons shown by us seems a bit too wide.
A lot of people would rather have a smaller distance between the icons.

[how]
Top bar buttons have a given padding. Additionally to that we pad the
icons a bit more. That padding can be removed, without violating the design
goals imposed by the top bar (i.e. menu button padding).

This makes the visual appearance a bit more pleasing. And still
conforming.

I'm sure people will want to further decrease the distance, but that
will violate the required button padding. A composite button would have
to be used then instead (like the aggregate menu does).

[why]
The spacing between icons shown by us seems a bit too wide.
A lot of people would rather have a smaller distance between the icons.

[how]
Top bar buttons have a given padding. Additionally to that we pad the
icons a bit more. That padding can be removed, without violating the design
goals imposed by the top bar (i.e. menu button padding).

This makes the visual appearance a bit more pleasing. And still
conforming.

I'm sure people will want to further decrease the distance, but that
will violate the required button padding. A composite button would have
to be used then instead (like the aggregate menu does).

Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>
@Finii
Copy link
Contributor Author

Finii commented Mar 26, 2020

Let's examine the padding situation with looking glass:

This is the actual button we insert (image 1):
1button

This is the icon box we put into the button (image 2):
2icon

This is the icon itself (image 3):
3iconitself

Usually a button has a bit padding between its text/image and the left/right border. This is the padding you see between the red borders in image 1 and image 2. The distance is a system value, and it is used throughout. Like with the composit button here, note the distance between red border and actual icon:
5padding

But the padding intoduced between image 2 and image 3 is not needed. We could actually put 'just the icon' (without additional padding) onto the button.

Here a comparison of before and after:
6pre
8post

@Finii Finii changed the title appIndicator: Remove unneeded padding feature: Remove unneeded padding Mar 26, 2020
@Finii
Copy link
Contributor Author

Finii commented Mar 26, 2020

This might solve:
#217 #200

I remember more comments about the icon distance / padding, maybe make it adjustable etc, but can not find that right now.

@Finii
Copy link
Contributor Author

Finii commented Mar 26, 2020

Maybe interesting in this regard... As already mentioned in #78:

There is an extension to change the system wide padding:
https://extensions.gnome.org/extension/355/status-area-horizontal-spacing/

Together with this PR you can pack the icons until they tough each other.

@Finii
Copy link
Contributor Author

Finii commented Mar 26, 2020

Please hold on there is a problem with at least antimicroX, reported by @terencode.
Here on my machine:
Screenshot from 2020-03-26 17-20-23

[why]
We remove all style settings from the gnome-shell.css that usually
affect the menu buttons. This includes the default icon size.

When the icon has some 'other' size, this will leave us with garbage.

[how]
Just overwrite the padding of the system gnome theme. Keep all other
style settings intact (including icon size).

[note]
We do not query the current style to build the new style, because (at
least at the moment) no other style setting is done anywhere. When in
some point in the future a style is set before this new line (which in
unlikely, because it is at the top of the constructor), we would have to
assemble the new setting more carefully.

Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>
@Finii
Copy link
Contributor Author

Finii commented Mar 26, 2020

Modifying the style now a bit more careful ;)
Should work now. here on my machine:

Without this PR:
previous

With this PR:
new

@terencode
Copy link

It's better but now the antimicrox icon has a fixed size, so it doesn't react to icon size change.

@Finii
Copy link
Contributor Author

Finii commented Mar 26, 2020

How do I change the antimicroX icon size?

@terencode
Copy link

terencode commented Mar 26, 2020

How do I change the antimicroX icon size?

Sorry, I meant using the icon size setting from your previous PR .

@Finii
Copy link
Contributor Author

Finii commented Mar 26, 2020

Did you manually merge/rebase both PRs? Because they are separate. Maybe you have a stale settings window still open?
I will try a merge of both PR and see if it works (but I guess you just switched to #219 with the settings dialog from #195 still open, which is not supported (and wont be there once you close it) in #219.

@terencode
Copy link

I checked out master and then merged #195 and then #219, no windows was open and then I restarted.
All the other icons except antimicrox resize correctly when using the icon size setting.

@Finii
Copy link
Contributor Author

Finii commented Mar 26, 2020

ACK. Icon size set to 8 or so. Antimicro icon does not shrink,

Screenshot from 2020-03-26 18-05-24

@Finii
Copy link
Contributor Author

Finii commented Mar 26, 2020

Fixed in #196. Thanks for the input.

@terencode
Copy link

Perfect :)

Copy link
Collaborator

@3v1n0 3v1n0 left a comment

Choose a reason for hiding this comment

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

Thanks for the research and testing, this was something I wanted to do as well, and looks fine without unsetting other upstream parameters.

So fair enough.

@3v1n0 3v1n0 merged commit 32f2f34 into ubuntu:master May 18, 2020
@Amathadius
Copy link

Amathadius commented May 20, 2020

There's no more space between the appindicator icons and the extension does not respect the general padding.
image

@Finii
Copy link
Contributor Author

Finii commented May 21, 2020

Hmn, the only thing removed was the extra icon padding. Around that is the button padding, which looks like to be reduced to zero in your case. For ex with this https://extensions.gnome.org/extension/355/status-area-horizontal-spacing/

This extension does respect the general top bar button padding.

Sure you did not yourself set that general padding to zero? The other buttons look like it.

@Amathadius
Copy link

You added a variable that forces the system-status-icon padding (or any padding except the hpadding between elements) to be 0. On my screenshot, I set the -natural-hpadding and -minimum-hpadding padding of the theme to 0. If I understand correctly, the (most) elements of the panel use "two padding variables" at the same time: system-status-icon and -natural-hpadding/-minimum-hpadding. Every element of the panel (except the system menu) uses both at the same time (one then the other). You add a variable that forces the system-status-icon padding to be 0, so the padding is forced to be 0.

But in normal use (if I don't change my theme so that the system menu icons are spaced apart like the other panel items), there is still a lack in the appindicator padding.

@Finii
Copy link
Contributor Author

Finii commented May 24, 2020

or any padding except the hpadding between elements

Hmm, do you mean the button padding with 'hpadding'? See the padding analysis I did in the second comment.

Screenshot from 2020-05-24 17-05-54

What has been removed is the icon box padding, shown in image 2 above.

And as you said, you set the hpadding to zero in your theme, which is maybe the same what https://extensions.gnome.org/extension/355/status-area-horizontal-spacing/ does. Then you get zero padding ;)

But in normal use [...], there is still a lack in the appindicator padding.

I'm not sure I understand this correctly without an image. The goal of this change was to have all appindicator icons grouped more closely mimicking the aggregate (system menu) button.
Maybe use looking glass' eyedropper (examintaion) tool to show what you mean?

Do you have any suggestion how to solve this clash of interests, so that the padding is not extremely wide for people that use the default paddings, and people like you that turn that hpadding value to zero and expect a button padding to exist?

I mean, except from introducing a option switch. That would of course be very easy with the still-pending-a-comment PR #196.

Edit: Correct PR number

@Amathadius
Copy link

Amathadius commented May 24, 2020

And as you said, you set the hpadding to zero in your theme, which is maybe the same what https://extensions.gnome.org/extension/355/status-area-horizontal-spacing/ does. Then you get zero padding ;)

No, not really. In my screenshot, the padding was provided exclusively by the padding value of :
#panel .panel-button .system-status-icon { icon-size: 16px; padding: 0 3px; }

Value that is used by default by all panel elements (including the system-menu). However, individual icons in the system menu do not use hpadding. It is used by the entire system-menu. As a whole.

The line this.set_style('padding:0'); seems to put no other padding than hpadding and therefore, the result is not the same as for the other elements of the panel (system-status-icon + hpadding + system-status-icon).

I made screenshots by exaggerating the values (the teal lines represent the hpaddind and the pink ones represent). The strokes are not adjusted to the nearest pixels mainly because the "white" of the icons is not always the same size (even if they are all 16x16 icons).

(Almost) Normal : hpadding 10px, system-status-icon : 10px
image

Hpadding : 0px, system-status-icon : 10px
image

Hpadding : 10px, system-status-icon : 0px
image

I guess the goal is to get the "same" distance between appindicators and the same distance between appindicators and the other elements of the panel (except for the individual icons of the system menu) and not to recreate a block as with the system menu. If this is the case, before, it was indeed too wide, but now it's clearly missing some padding and therefore the result is not the same as with the others.

With the "old version", I had added the line #panel .panel-button .appindicator-icon { icon-size: 16px; padding: 0 Xpx; } in my theme file to adjust the padding value to match the other elements (in my case, it was 1px more than the system-status-icon padding value, but I didn't have any hpadding. So yes, with a "normal" gnome theme -- small system-menu aside -- there was too much spacing before), but now appindicators don't respect this value, nor the system-status-icon padding. This is where I said that the extension doesn't respect the padding anymore.

Edit :
In fact, now that I'm re-reading all this, if I understood correctly, the problem with the impression of too much spacing with appindicators is due to the fact that if it's the only thing on the panel (with the system-menu), it does look too spaced, but only because the system-menu, by default, is condensed. The problem we're talking about and that we wanted to solve this merge resquest is not the right one: it's not the appindicator spacing that's the problem, but the fact that the spacing of the system-menu's icons is not the same as the spacing of all the other elements of the panel (including the icon block, the whole, of the system-menu).

@Finii
Copy link
Contributor Author

Finii commented May 25, 2020

Very good input, I will need some time to digest that :)

I fully agree with your Edit paragraph. But still, it bothers people and I guess what they really want is a looks-like-sysmenue thing - although that is only one button with several icons rather than individual buttons.

@Finii
Copy link
Contributor Author

Finii commented May 25, 2020

Hmm, @Amathadius has a point here.

When people use also other externsions that reside as icons in the top bar (printer, coffee, and key in the example below), they are all the same 'too wide' width, that we had before this change:

Before, equal spacing:
original

Now, more condensed spacing for appindicator icons:
new

(Ubuntu 20.4 unmodified)

I still find it strange to set the hpadding to zero in a style and then complain about padding-less icons ;)
Further for example Places Status Indicator, as it is not only an icon but text, has no icon padding, just the hpadding. If hpadding is set to zero, that would also stick to it's neighbors.

My conclusion would be:

  • Removing the icon padding looks indeed strange when other externsions' icons are shown next to it that have icon padding
  • Still this solves 'problems' people have with the wide spacing, in most cases as it seems
  • I'm not sure Amathadius' issue is really an issue, or just a corner case, that fails also with other indicators that show text-and-dropdown-icon' like 'Places Status Indicator' or also the system's 'keyboard language chooser'.

Untitled

Could you @Amathadius show the system keyboard language chooser thing icon with your zero padding setting?

@Amathadius
Copy link

I can show you how I did with the panel extensions that are only text (in my first screenshot, there's the Hamster extension example), but the question is mostly how the "flaw" or lack of uniformity in the spacing of the gnome panel elements (the system-menu icons being concidered as a single element, hpadding works only for the block, which distorts the uniformity of the panel) concerns this extension. The extension creates individual elements that are governed by the same rules (of the theme) as all the individual elements of the panel. That's it, I think.

If I don't like the panel color, I'm not going to hardcode a panel color into an extension that has nothing to do with it. It's the theme that does that. Same thing for the spacing of the panel elements.

@Finii
Copy link
Contributor Author

Finii commented May 26, 2020

In the top bar, a lot of the extensions show a icon in a icon box in a button.
But some show something else in the button. For example text. Or text and a dropdown icon.

The button has padding.
The icon box has padding.

So often the extension shows this icon in a icon box in a botton, so you get padding from the button and padding from the box, and in the center is the icon image.

To get a tighter spacing, we take the liberty to not put the icon in a (padded) icon box. This is the same as extensions with text or with text-and-dropdown-icon do. Of course you can say we do not respect the global padding, but... we do. Just unconventionally, although the button hold just an icon we do not put it in the padded icon box. This is kind of taking the liberty to so something not so conventional, but also not totally strange.

Rather, I believe your totall lack of hpadding is the problem, it does not work with this extension very well, but it also fails in a lot of other places.

I prepared four screenshots of my system. From top to bottom:

  • My normal system, appindicator icons in icon boxes (prior to this PR)
  • My normal system, appindicator icons directly on button (this PR)
  • Your system, appindicator icons in icon boxes (prior to this PR)
  • Your system, appindicator icons directly on button (this PR)

all

When I look at the bottom most two versions, that show your system, we can see what you complain about. The icons touch is the new (bottom) version, but are nicely packed in the old (2nd from bottom) version.

But please note, your setting of hpadding: 0px makes problems in a lot of other places too:

  • Activities and Places touch
  • ping extension and keyboard language touch
  • Basically all items in the top bar that are NOT in icon boxes fail to have any separation

all2

My conclusion is still:

  • We do something not strictly conventional here, but it fixes visual problems of some
  • You expect this to work where other extensions also fail the same way (because you set a very strange value)

Note:
To mimic your system I set

#panel .panel-button {
  -natural-hpadding: 0px;    
  -minimum-hpadding: 0px;

@Amathadius
Copy link

For extensions that only contain text, I adjusted the left-right padding of #panel .panel-button StLabel to be the same as the padding of #panel .panel-button .system-status-icon. So, obviously, for extensions that contain an icon and text, it's a bit weird, but since I only have the Openweather extension like this, in the middle, it's not that bad.

But then again, we're talking about adjusting the theme (or the theme style), not adjustments in the extension's code.

You can create a style sheet for the extension, if you want (although, again, what's the point of modifying that), but within the coding of the extension...

Of course you can say we do not respect the global padding, but... we do. Just unconventionally

I think I've already demonstrated that no, it doesn't respect the conventions at all. If it respected the convention, you wouldn't need to force a theme padding from the extension. If every extension did that, we'd have a problem. Suppose one extension wanted to have more padding than the others... Forcing a 0px padding is like forcing a 10px padding. No, it's not conventional, even unconventionally. It just doesn't make sense.

Anyway, the real problem (and it doesn't concern this extension) is the fact that you can't (or I don't know how to) control the spacing of only the system-menu/aggregator menu icons. It actually becomes very apparent when you only have the appindicators and the system-menu (in that case, yes, the appindicators seem to be spaced further apart), but I think I would repeat myself saying that it doesn't concern this extension at all.

@Finii
Copy link
Contributor Author

Finii commented May 27, 2020

My 'design idea' was to create a button in the top bar, that is not an normal/ordinary 'button with a system status icon', but rather a 'button with an image'. That image can then be displayed any way we want, for example without padding. So my justification/excuse was: This is no 'system status icon' and thus we do not not-respect any theme settings, because they are not valid for our 'image of an icon' on a button.

As I pointed out earlier, I think you have a point. Still one needs to decide if 'bending the expectations' is okayish here, if it makes a lot of people happy.

In the above posts I spell out all my reasoning behind it detailed enough, I think.
Personally I do not care either way. I can help you prepare the PR to remove this feature again gladly. As gladly as I tried to help the other (numerous) people that wanted less padding. If you would like a PR prepared, I can do that for you. At least you should maybe post something in Issues instead here in the already pulled PR.

Maybe a resolution would be to make this an option setting. Means mixing it in with PR #196 for example. People that can not or do not want to fiddle with shell themes can at least easily achieve what they desire. But unfortunately @3v1n0 did not comment on that PR recently, and it just hovers there. That other PR is where my heart hangs, and imho it improves this extension a lot. Anyhow.

To be honest, I'm torn with this here. On one hand you are (somehow) right, on the other hand if it helps many and only one is offended ... ;)

In the end 3v1n0 has to decide (or some other maintainer if there is one); I'm just an ordinary user of this extension that created two PRs :-D I think your reasoning is sound (as is mine), and someone has to take the plunge of a decision *grin*.

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.

size and padding issue / use with topicon / looking for manual override
4 participants