- User Since
- Mar 30 2016, 2:05 PM (198 w, 4 d)
Thu, Jan 16
Can the top of the scrollbar go below the column headers instead of next to them?
As a side note: I have been using this code for a couple of days now, and while I think the separator looks well for vertical scrollbars, when you have both vertical and horizontal, ... this will need some getting used to.
This is with a thin handle, but having the thick one does not really improve. The view looks like a frame inside a frame, first one being off-centered.
I have no clue how this can be improved
Wed, Jan 15
Confirmed that the centering is fixed. Now the arrow seem to have moved vertically (for vertical scrollbars) by half a pixel, which is probably a consequence of the 20->21, and make them look somewhat thicker due to antialiasing. It is a minor detail, so feel free to ignore.
Other than that, no more comments !
yeah thanks !
Now, it turns out that the arrows are still ill-aligned (with respect to the handle). Maybe they use the full width for rendering ?
Alignment again: so you increased the width of the groove rather than increasing the width of the scrollbar. This indeed fixes the alignment of the handle (and groove), but now the scrollbar arrows are off centered. (for the same reason), with both the scrollbar groove and with respect to the line. I would revert to the old handle width and rather increase the scrollbar width by 1 pixel. Then adjust the positioning. Does that make sense ? Increasing the width of the arrows by one pixel might break pixel alignment and create inconsistencies with other places were arrows are rendered ...
Now I realize that the thin scrollbar will still be off centered (and always was so far) while it is well centered with your patch, since it is only 3 pixels while the thick one is 6 pixels ... This was not visible without the separator line. But it is going to dissapear with the other patch anyway. (otherwise one would probably need to increase its width to 4 pixels)
Hi again Marco,
since you mentionned centering in plasmoids in the other commits, in fact here also the centering is wrong, with the added separator. Probably the offset is the one pixel you take for the separator. Can you double check ? (this appears true for QWidget based scrollbars, not QQC). Likely you need to increase the scrollbar width by 1, and tune the positionning of the groove+handle.
Tue, Jan 14
my complain here is that this argument was either not made or discarded, when the first switch to thin scrollbar was done.
This is my main concern about this change: the going back and forth using adhoc arguments each time to justify the change, often contradicting each other. It means essentially that we either don't know what we are doing, or dont think our changes enough. This is bad imo
If I had been around back then or noticed the patch, I would have made this argument.
I would start by
- rebase this patch to apply cleanly
- split it in two (one for the separator line and one for the think handle removal), because these are really two features, adressing two different issues:
1: the floating bar
2: the two small handle that few people dislike.
Removed previous comment because after testing the patch it turns out that this is not too strong of an issue.
However I also noticed that you not only removed the think handle but also the color blending. This make the handle not only larger but also stronger in terms of colors, especially in the case where the parent list has focus (you get a strong blue handle).
Is this really intended ?
I would at least keep the color blending, making the color stronger only when the scrollbar is actually hovered.
With all due respect:
This is irrelevant unless you do an actual poll.
You will have user feedback that want the thin scrollbar back. (I will).
you need to rebase (you have some unrelated changes in the diff, which revert latest change from Noah)
- Concerning the separator, it looks great.
- Concerning reverting the thin bar, I think this is really unfortunate. I expect you will have as many complaints after the revert for people to have it back, as now of people who want the thick one. I would be among the former. Going back and forth in these design issues really gives wrong impression IMHO on our design capabilities, and I think this would be one of VDG's task to avoid this at all cost.
Now, the thin bar might not work very well with the new separator (and both pursue opposite goals: one to make the scrollbar more visible and the other less visible).
But then again, it just means that we, (including our designers and usuability experts), keep going back and forth on which direction to follow (here regarding whether we should make the scrollbars more prominent or less prominent). This does not give a good impressing to the outside world IMHO.
- In QQC there is an issue of overlap between item selection (and header titles) and scrollbar. Isn't that made even worth with the addition of the thin separator ?
Mon, Jan 13
I'm confused: it has happened, no ? If I apply the patch I now get a white background (with normal breeze color scheme), behind checkboxes and radio button, always, which was not there before. This is , I think because of the change at line 3889.
in more detail: imagine a color scheme where window background is black, window text is white, view background is white view text is black.
you will get a white square on a white background for unchecked checkbox ...
I would strongly object to that:
first that is a visual change unrelated to the issue this patch adress
second this is semantically wrong, and this will break on some color theme.
Things should be kept consistent. If you change the background role, you must also change the foreground role, and then you might really get funny results.
ship it. No strong feeling.
Sat, Jan 11
I would prefer not systematically render the background, because it might break existing applications, like rendering a widget on top of a painted image. Also this goes against Qt's rendering model which does not require rendering widget background, relying on that of the parent widget or window itself. I would instead either do it in qqc, or detect if we are rendering a qqc control and in this case only render the background systematically. There are several places in the code that do that already. Look of "#if BREEZE_HAVE_QTQUICK" blocks.
Sat, Jan 4
Mon, Dec 30
Sun, Dec 29
if QWeakPointer::data() is obsoleted, (while needed in the code), I would object to adding toStrongRef, in between, since as pointed out by Antony, calling data immediately after brings no further security, and just results in some overhead (Weak to shared pointer). It means that either
- weakPointer::data should not be obsoleted
- the kdecoration api should be changed (to return shared pointers, or provide direct access to the data).
Fri, Dec 27
looks good. See comment about the symbol pen width change and then ship it.
Dec 19 2019
Looks good and sensible. Very nice consolidation. Thanks !
Dec 17 2019
Just a remark: WASD is very keyboard layout dependent. For AZERTY keyboards (as used in France for instance) this is completely broken, and something any gamer has to always change manually (to ZQSD) if not already done automatically by the game (which may games do)
Arrows on the other hand are much less layout dependent.
Dec 14 2019
As a side remark: did you check how it looks when one selects "text under icon" or "text beside icon" for toolbar button ?
Fix is legit. Thanks !
I would avoid changing the alpha of the color though because: it makes no difference, and it is unrelated to the issue. Should be a different commit (which you can do without review if you really think it is important)
Dec 13 2019
Dec 9 2019
I'm not sure what you mean when you wonder how it would play with monochrome icons. It should have nothing to do with them unless you embed stylesheets in the SVGs to use the separator color in breeze icons.
So the background here is that we've gotten a new VDG designer who produced mockups of Breeze dark with dark separators, and some people absolutely fell in love with them, while other people hated them. We could not achieve consensus on moving to use dark separators for Breeze Dark, so there was a desire to offer people that choice. It occurs to me that we could put this in the Breeze style's own settings, if it's strictly necessary to expose this to users. Personally I would prefer to just make a decision on separator colors one way or the other rather than making it explicitly configurable (dark separators FTW :) ).
Few more comments on this:
- general: you will never be able to make all the opiniated people happy, and you have to draw a line (otherwise your code will become bloated, buggy, and unmaintainable)
- regarding this specific case: many widget style will not implement this new color. For those this will just generate bugs reports: why is my color scheme not respectd ?
- some widget styles (oxygen at least, but I'm sure there are others) use two colors for frames and separators, to mimic shadows or relief effects. Adding one single color for this will not fit such schemes.
- in the end if you need an extra color for a given theme (be it future-breeze or whatever), there is also the possibility to add it as a extra option for this specific theme, rather than forcing it to kcolorscheme and imposing it to all styles (or making kcolorscheme broken for all the styles that wont use it).
This is how window decoration shadows and glow were handled to oxygen.
I think this change will break more things than it will fix, especially if the fix is to make some opinionated people happy.
Would also be good to have the opinion of @cfeck on this.
Adding new entries to the kcolorscheme should be done with a lot of care, because it could be seen as some sort of API break for existing colorscheme, as soon as you start using this color in the widget style: you would need a fallback implementation, for all the colorscheme in the open which do not implement this particular color. These kind of additions happened a lot whith gtk3 css style and resulting of overall unhappiness and distrust from theme/color scheme developpers. This also increases the complexity of the code and difficulty to maintain.
Dec 2 2019
Nov 28 2019
Nov 27 2019
Also, this change will make it pretty difficult to use git blame and git bisect in the future to track possible regressions. (to my knowledge at least). Might as well start a new repository.
Also (and somewhat independently from this patch): with Qt6/KF6 being around the corner, I somewhat wonder about the utility of rewritting and hacking breeze at that time. Would this make sense to actually leave breeze (which is a rather well-working theme, with very little bug reports) unchanged, and start working anew on a new widget style targeting KF6 ? Would that not be more exciting, bring more people in, trigger new ideas, give more freedom ? And be a more ambitious task all in all ?
One could of course start from breeze, and apply all the new ideas about how the code should be organized, how highlight should look, and checkboxes, without disturbance for something which has lived largely unchanged (and with not so many complains) for all the kf5 lifetime ...
You are missing copyright information and license in all the newly created files.
On the review side: it is impossible to actually review, right ?
Nov 13 2019
The question was already raised in the past. At the time there strong objections against this, some of them raised by Martin (now added as subscriber) and to which I agreed, but which I can't remember today. @graesslin any recollection ?
Oct 7 2019
Sep 18 2019
on the other hand, after checking that the dials keep the old appearance when "wrapping" is turned on, and since dials are rather seldom used anyway, I have no strong feeling against the change (still prefer the old look though)
Problem with the new design if you ask me is that it does not convey the information that you can roll around past the maximum as in a circle anymore.
All other widget styles (except now breeze), use a circle metaphor for a dial ... Personally I think the new look just looks ... broken. It looks like a bent scrollbar, (or a bent slider), which a dial is not ...
Aug 28 2019
for what its worth: to me smart means: there has been some thought put in this placement policy and it should be the best one for you. If you don't like it, you might want to try the alternatives. When I read "smart" as an option, immediately I think "that's what I want", and select it, if not already selected. In other words, it is another (better) way of saying "default", without having to go through the actual description of what it means. And then of course, I must trust the devs that "smart" is indeed smart, and is indeed what I should want, even if I don't understand it.
Aug 21 2019
Aug 20 2019
Aug 16 2019
Zoom showing the issue mentionned above with the "current" patch (or master branch of breeze)
Thanks for the patch, however, it is not the right fix to the issue. If you use a light color scheme (like the default breeze), you will see that the shadow below the part of the button that corresponds to the arrow is darker than below the rest of the button. This is because the frame is actually rendered twice.
Aug 6 2019
Aug 5 2019
"KPageWidgetItems live in a multi-page KPageDialog"
Is that really true ? Does it not live also standalone when running e.g. kcmshell5 kwindecoration (or any other kcm ?)
If yes, and at least in this case, the title has to stay, because there is no side bar.
Aug 1 2019
Thanks for the fix !
Oxygen needs the same patch. Will you also push it there ?
Jul 26 2019
Jul 25 2019
Jul 22 2019
Jul 20 2019
Patch works like a charm in standard config dialogs.
As a somewhat off topic remark though, it does not work in kcmultidialog (in framework kcmutils), even though it derives from kpageddialog.
This shows up for instance in the "window manager settings" you can get from the decoration menu (Alt+F3), or simply when running oxygen-setting or breeze-settings.
Looking into kcmultidialog, it seems there is some internal handling of margins in there, for a reason unknown to me. Maybe you want to investigate there too (in a different patch)
Jul 19 2019
Jul 15 2019
Personal opinion follows.
Few words about colors (sorry if you know all about it already)
Jul 14 2019
- For the record: toolbar buttons (or rather QToolButtons) can have focus too, when they are used outside of toolbars (and there are examples of such everywhere, for instance when you have an "open" button next to a text entry for selecting a file.
your enumeration did not mention
- checkboxes and radiobuttons who also have hove/focus + the various selected states.
- tabbar entries (hover, focus, selected)
Jul 5 2019
Simplified the patch: special case in ::pixelMetrics is in fact not needed, because it is overridden in ::frameContentsRect anyway
New patch, adding proper margin to avoid overlap with viewport
Indeed. This is a problem. As soon as treeviews are animated Qt makes a pixmap copy of the treeview viewport. This does not include the blue line on the side, which is painted in the frame, below the (transparent) viewport. Will investigate further.
Jul 4 2019
This is not what I said. What I said is "thanks, it looks fantastic" is not a proper review and hence irrelevant. What I expect for a proper review is
- does the patch achieve the behavior matching its title, and is it the intended behavior
- does the code look legit (compiles, properly formatted, etc.)
- is there a better way to achieve the same
- does the patch break other things, and how can it be improved so that it doesn't
If you can provide such a review I'd be happy to modify the patch accordingly.
Jul 1 2019
(I also included the white background in there, because it turned out a bit tricky).
Jun 28 2019
ather simple and attached. Feel free to add this or something similar to any other modification you plan to do.
Thank you very much Hugo.. that pixel was driving me mad ;)
Do you want to push it yourself or I just include it in the breeze modifications I'll do for the sidebar style?
Jun 27 2019
On the breeze side, asside from the layout margins/spacing issues, there is at least one hard-coded pixel in the frame rendering that must be removed. In fact I have this committed in my local breeze version already. The corresponding patch is rather simple and attached. Feel free to add this or something similar to any other modification you plan to do.
Jun 7 2019
Jun 3 2019
May 14 2019
May 13 2019
So, how does that work when user select a specific size, and then change decoration ? Is his choice erased by the default of the new decoration ? Or is his choice maintained (in which case he never gets a chance to actually see the "preferred" width of the new decoration ? And then, if erased by the new default, does that mean that if I switch from decoration A (with custom setting) to decoration B, and then back to A, my custom setting is lost ?
Apr 3 2019
Did anyone check how this patch look with other icons than those in the screenshot ?
E.g. the preview icon in dolphin, or list sorting or search icons ? Does the new arrow overlap with the said icons ?