kotelnik / plasma-applet-active-window-control

Plasma 5 applet for controlling currently active window.
GNU General Public License v2.0
119 stars 18 forks source link

Visual improvements for app menu #66

Closed psifidotos closed 7 years ago

psifidotos commented 7 years ago

some visual improvements for app menu and when app menu is combined with window title

image

kotelnik commented 7 years ago

I've reviewed your code and decided to just extend options to match the state you wanted to achieve. Some of your changes would break some configurations so I wanted to do them less invasive (and configurable). If you find that something is still missing, feel free to let me know. Anyway, I'd like to say many thanks for your code!

psifidotos commented 7 years ago

@kotelnik thank you very much for considering the changes!! :)

I am using the master branch and I am satisified. I would like your opinion on the following (this is what I miss) :

  1. When the window title is appearing with its menu, I prefer the window title to become bold, it is an easy way to distinguish the title from the menu
  2. You kept the top alignment for the menu bar (instead of the bottom one) but even this way it is very difficult to just use the menu font scaling in order to achieve, same font size with the window title and correct vertical alignment with it
  3. I like that you kept the separator line between the window title and the menu bar. The only small issue I have is that in order to be in the center of the distance between title and menubar needs a very big menu side margin. I think that the separator line should be always in the center of the distance of window title and menu (this is not possible for menu side margin lower than 20px. in my system)
  4. the previous layout of the separator was exactly the one used by the audoban's separator (which is the only separator plasmoid available :) ). On the screenshot below you can observe that the active window separator is distinguished from all other panel's separators... If I remember correctly, the audoban's separator uses:
    • always a top and bottom margin of 4px. and left and right of 3px in horizontal panels. (this is why is used 24 and 23 in the previous commit)
    • opacity at 0.5

thanks a lot for this fantastic plasmoid and I am considering it one the biggest community contributions in kde !!! :)

image

kotelnik commented 7 years ago

Hi! Thanks for the praise :). And thanks for the good suggestions!

ad 1) Implemented (new option in App menu settings) ad 2) I've implemented new and appmenu buttons - it should fix the issue. ad 3) I think it actually is in the centre if you take into account the button border (only visible on mouse hover). But maybe I haven't tested your particular use-case... ad 4) OK, now I understand why You implemented the separator this way. I was not aware of the separator widget. I've cloned the separator present in Plasma built-in Digital Clock widget. From my perspective this is the reference one since it is from Plasma authors themselves... I try to contact audoban and suggest to follow the Digital-Clock-style separator. Hopefully we will unite this thing :).

Let me know if there is still something missing.

psifidotos commented 7 years ago

@kotelnik I checked latest master... everything looked ok... the only 2 things I changed in my system:

  1. the separator but this is already discussed... personally I dont take sides anything that creates uniformity (even though I prefer audoban's design, I would prefer the plasma applet to follow audoban's design)
  2. the menu buttons width to: width: appmenuButtonTitle.implicitWidth + units.smallSpacing * 3

instead of *_2_** for the smallSpacing, ift feels more natural in my system

that's all... very good work!! ;)

kotelnik commented 7 years ago

Thanks! ad 2) You are right, it feels better. Done. ad 1) I think both designs have good ideas. Audoban's has radius and better visibility, Plasma's uses margin based on fraction of panel height rather then fixed px. We'll see where it goes. Sorry that I went the Plasma way for now :).