- User Since
- Mar 30 2016, 2:05 PM (102 w, 4 d)
Fri, Mar 9
I am confused. Where is the shadow rending code ? How is it shared between style and deco ?
In general, the sharing should be a separate patch, that could be applied even with the current shadows, before modifying how shadows are drawn ...
That would make a two steps patch: first share shadow code (with existing shadow) between kstyle and decoration, then modify shadow code.
Thu, Mar 8
- Do we really need different shadow sizes for menus, etc? Can we use single shadow size in kstyle?
Tue, Mar 6
- I agree with Nathan that the bottom part is likely too hard.
- I think the same shadows should be used for windows and menus
- Also, thanks for posting the pictures. Is there any chance you could also post new vs old shadows side by side for the different sizes ? I have the feeling that for a given shadow size the new shadows appear larger ...
Tanks in advance !
Mon, Mar 5
Thanks for the patch.
- As we discussed already in telegram, I am not convinced you absolutely need fast fourier transform nor blur, and could probably handled thing with "simple" QGradients.
If you do not have time to try implement such a solution, I can give it a shot myself whenever there is time.
- I can also help with moving the loading and destroying of the shadow to the pluggin loading/unloading rather than to first and last window opening. (this could also go in with the current shadows in fact, and would rather be a separate patch).
- Also, these shadows look nice but are rather strong. To me they represent a shift of paradigm with respect to the original (subtle) breeze ideas. But that is not me to decide. Still: could you also post screenshots with the smaller sizes ? And then we can discuss what the default size should be.
- finally, I really don't think we need too separate setting for the two shadow strength. These are just implementation details. IMHO. And will make the code very hard to maintain. (how do you test that all combinations of settings look good ?)
Others opinion welcome, of course.
Sat, Mar 3
ok, ship it, then.
In case we get complains, we'll increase the splitter size a posteriori.
Mon, Feb 26
yeah so ... Qt Broke it. Was not broken in past version (not with the version I have) (5.9.4)
Note that what is broken is the rendering (basically Qt having issues with transparent widgets), not the actual functionality. (the ability to resize splitter in a region that is larger than the actual splitter width).
Sun, Feb 18
Sat, Feb 17
Feb 17 2018
Ship it !
Thanks for this patch, and sorry for the mess on the Blur patch (should have used arc).
Feb 16 2018
Ship it !
Thanks for the patch.
I "think" one must also do a similar fix in "Style::menuItemSizeFromContents".
It seems to me that there also the line 2728
leftColumnWidth += Metrics::MenuItem_ItemSpacing;
should also be conditionned with "if iconWidth > 0"
Can you double check ?
Feb 15 2018
ok. No reaction on the UI.
Ship it !
Thanks for the work and the patience.
Thanks for considering my hesitations on this.
I do think this change is either not mature or not needed and should be reverted.
I do not think this revision should be abbandonned just yet.
The part that takes care of removing double spaces when icon is not present (when no icon is present in the menu), should go in.
The part about increasing the side margins to make the checkboxes centered, should still be discussed: It is not clear to me that margins and spacing between items should be identical, and for instance see how the positioning appears off with respect to the bottom of the menu, when checkboxes is present on the last item of the menu.
One solution to fix this would be to also increase the bottom margins, but then in turn that makes the item selection funny. (asymetric top bottom). Unless of course one either
- make the selection not extends to the end of the menu any more
- increase the spacing between items to restore the symmetry.
Ok. This time I am strongly against this change.
This vertical line serves no purpose, clutters the ui, and is not "simple by default".
Why would one need to separate the checkbox from the icon and text to which it is directly related to.
Also see how it breaks with separator and item selection.
To me it is a no go.
(Note that I am not happy with any on the later community accepted commits to breeze either: blur serves no purpose either, nor the extra space allocated for checkboxes, etc.)
If people insist on this getting committed, I will oblige, and resign from maintaining breeze at the same time, for the reason that it is going in a direction which I do not like. I cannot maintain a code which renders to something I do not like (nor understand).
Feb 14 2018
Why are there only 2 pixels?
Feb 13 2018
Ok. So, the patch works, but ... there is something fishy here. In principle it should not be needed. Right ?
Ifcheckboxes have a fixed size, and if margins would be equal to spacing, and if the math are correct, having something laied out as: margin + checkbox + spacing + icon + spacing + text would be automatically centered, no ?
Idem without icon. (and if there is double spacing, then it is a bug).
At least this is how the code is supposed to work. If not, it should be fixed, rather than circomvoluting the issue with some centering.
(note that the padding on the icons is disconnected to this: not all icons have padding, especially if you change icon theme, so you cannot do anything but rely on the icon size.
So ... I applied the patch (which I approved), used it, and ... don't like it sorry.
See attached screenshots
I think the unnecessary space breaks alignment with the menubar above. and is completely unnecessary. Is this _really_ what we want ? @ngraham ? @alake ? @colomar
Also, code breaks in the following configurations:
the diff appears more complex than it actually is because of unrelated changes. Please keep the changes to the minimum, this will help reviewing.
Feb 12 2018
last round of modifications: simplifications of the blurHelper class.
Also, for the config ui, maybe it makes more sense to call the "Transparency" tab, only "Menus" and change "Menu:" into "Transparency:"
Unless of course one wants to add transparency behind other widgets.
also see how in two the posted screenshots the checkmarks for checkable items is actually very close to the menu border.
Feb 6 2018
Thanks for the updated patch and for the work on BlurHelper.
Couple more comments below.
Feb 1 2018
thanks for taking care of 1, 2 and 3.
Jan 31 2018
With all due respect, please stop "accepting" revisions. Revisions should be accepted by the code maintainer, who then endorse the responsibility that the code then remains in a workable state forever after. You are not the breeze maintainer.
In my understanding at least, accepting a revision allows the submitter to commit the code as is (as in "this revision is now accepted and ready to land").
In the current state, this revision is _not_ accepted and ready to land.
thanks for the patch !
Jan 25 2018
Jan 24 2018
Jan 20 2018
Jan 15 2018
Ship it ! Thanks !
Jan 14 2018
Jan 11 2018
Jan 10 2018
Checking the patch in action, it also indirecly changes color handling. In the previous implementation, focus color was used to render the 'non-mouse-over' scrollbar for the widget with focus. This is not the case anymore with this patch, because "disabled" is used unconditionally, when not mouse over. I am not sure this is a good thing. Maybe focus color should be preserved on the thin scrollbar.
That looks nice. I don't think it needs a separate settings, since once again the hit area is the same as before.
Then: would be nice to have VDG oppinion on that, and see if we can make the code less hackish (on the mouse-over detection side).
Jan 6 2018
Other than that ... Ship it ! I'm fine with the change and happy to maintain it in the future.
Jan 5 2018
Jan 4 2018
Jan 3 2018
Ship it. Thanks !
thanks for the patch !
Jan 2 2018
Right, that's exactly it. The old menu shadows were just hardcoded to use a smaller version of the old default size, and it didn't consult the default at all until you changed it, at which point it becomes as big as the menu shadows.
Don't think so. The same "default" shadow size is used for the window decoration and for the window style. Now of course, once the style has been recompiled you need to restart the applications to have the new default value properly accounted for. No bug. Just a momentary glitch due to recompilation I think.
A follow up on the menu vs window decoration shadow size, and a suggestion for the window decoration shadow size option, for discussion.
- right now there is an explicit 3/4 (12/16) conversion factor between the menu shadow size and the window shadow size, which I believe is working correctly, except possibly with update glitches, when the window size is changed.
Maybe this scale-down is too small especially since the new default window shadow size is larger, and one could for instance try 1/2 (or even 1/4th) instead of 3/4
Jan 1 2018
Hello, my take on the following suggestions:
Dec 31 2017
Hello Nathan, thanks
I have no objection to the change, in principle.
- I feel that the top shadow at least should be somewhat smaller than the three other sides. Comments welcome
- could you check with ALake. He is also VDG and the original style/decoration designer. If he already gives his approval on any other channel (e.g. Telegram), then fine with me too.
- there is another review (I think, don't remember which), on the same topic, and which should be discarded if this one gets landed.
- Did you check that there is no *glitch* with the shadow maximum size, on small size tooltips' shadow. (last time I checked there was one, which I had no time to fix).
Dec 12 2017
Side note: feel free to push the same thing in the oxygen repo.
Thanks a lot. Works for me.
Dec 11 2017
I agree the text of the option could be made better. However, the formilation you suggest make it sound as if you cannot resize maximized window without checking the option, which is not quite true: you can use the window menu, shortcuts, in case of quick-tile you can drag the window away from the corner, and then of course you can unmaximize and resize.
The other thing is that "display window borders" is confusing since in breeze (as in oxygen), window borders are invisible.
Maybe better something like
"Allow resizing maximized windows from window borders" ?
Dec 10 2017
Thanks for the patch !
Sounds correct. Feel free to submit and to backport to plasma/5.11
You might also want to check the close to identical code for oxygen, in case it also applies.
Dec 8 2017
Dec 7 2017
Dec 6 2017
I thought (but might be wrong) that you could create an icon with already the right pixmap in there (QIcon::addPixmap( QPixmap, QIcon::Mode, QIcon::State )) for a given state and mode, which will get picked (as opposed to calculated on fly) whenever called.