hpereiradacosta (Hugo Pereira Da Costa)
User

Projects

User does not belong to any projects.

Today

  • Clear sailing ahead.

Tomorrow

  • Clear sailing ahead.

Saturday

  • Clear sailing ahead.

User Details

User Since
Mar 30 2016, 2:05 PM (217 w, 22 h)
Availability
Available

Recent Activity

Thu, Apr 30

hpereiradacosta added inline comments to D29254: [RenameDialog] Add an arrow indicating direction from src to dest.
Thu, Apr 30, 6:36 PM · Frameworks
hpereiradacosta added inline comments to D29254: [RenameDialog] Add an arrow indicating direction from src to dest.
Thu, Apr 30, 6:32 PM · Frameworks

Apr 7 2020

hpereiradacosta added a comment to D28651: Load and use global animation settings.

I have no idea how this is supposed to work. But the current fix overwrites what's in the breeze configutation, right ? Would it not lead to utter confusion ?
These settings should be set at one place and one only. Whether breeze or global I have no strong oppinion (so no strong objection to this approach), but then the other setting must go. (presently: the one in breeze)

Apr 7 2020, 1:59 PM · Breeze, Plasma

Mar 31 2020

hpereiradacosta added a comment to D27669: [kstyle] Tools area.

Hi
I don't think it is very satisfactory to say that the perforrmance hit it is "the apps fault" ... It is not there without the change, and with other widgets styles (or is it ? Did any one check, e.g. oxygen which is quite resource heavy because of gradients and so ?)
You can't expect applcations dev to go optimize their app for the need of a given style. especially if it is not trivial.
I don't see another choice, if such popular apps as kate and kdevelop are affected, than to go do the optimization 'ourself', if indeed this is the issue.
At the minimum someone should try to profile this, using e.g. callgrind, to see where the curlprit is ...

Mar 31 2020, 8:05 AM · Plasma

Mar 29 2020

hpereiradacosta added a comment to D27669: [kstyle] Tools area.

I've discovered another bug. When you move a window by dragging on an empty area, all hover effects stop working.

Mar 29 2020, 8:27 PM · Plasma

Mar 26 2020

hpereiradacosta added inline comments to D27669: [kstyle] Tools area.
Mar 26 2020, 4:29 PM · Plasma
hpereiradacosta added a comment to D27669: [kstyle] Tools area.

So I'm still having an issue with the feature um, not working. :( I have it turned on in the Breeze settings but it behaves as if off; that is to say, I see no different appearance compared to the status quo.

Is anyone else experiencing this, or just me?

Mar 26 2020, 3:09 AM · Plasma

Mar 25 2020

hpereiradacosta accepted D27669: [kstyle] Tools area.

All good, as far as I am concerned. Thanks !

Mar 25 2020, 10:42 PM · Plasma
hpereiradacosta added a comment to D27669: [kstyle] Tools area.

I finally got some time to look at the code. Some minor comments below (compiler warnings)
Also there is a problem with menubar text color when option is disabled. Here at least it is still set to the decoration color, leading to invisible text with default breeze color scheme.
See:


Mar 25 2020, 9:50 PM · Plasma

Mar 23 2020

hpereiradacosta added a reviewer for D27669: [kstyle] Tools area: hpereiradacosta.
Mar 23 2020, 7:35 PM · Plasma
hpereiradacosta added a comment to D27669: [kstyle] Tools area.

Hi Carson,
Thanks for the update. At first glance this all look pretty good. I should be able to do some more in depth testing and code review by the end of tomorrow.
Hugo

Mar 23 2020, 7:34 PM · Plasma

Mar 21 2020

hpereiradacosta added inline comments to D27669: [kstyle] Tools area.
Mar 21 2020, 9:03 PM · Plasma
hpereiradacosta added inline comments to D27669: [kstyle] Tools area.
Mar 21 2020, 8:50 PM · Plasma
hpereiradacosta added inline comments to D27669: [kstyle] Tools area.
Mar 21 2020, 8:47 PM · Plasma
hpereiradacosta added a comment to D27669: [kstyle] Tools area.

Nice, the colors are better now. I still see a difference in animation speed when the titlebar and toolbar change color though. It's especially visible with the current Breeze color scheme.

Mar 21 2020, 8:40 PM · Plasma
hpereiradacosta added a comment to D27669: [kstyle] Tools area.

Hi Carson,
Thanks for the updated patch and screenshot.
First I agree that the new (latest) checks on whether the toolbar palette was changed or not are much more elegant and just as efficient as the previous implementation.
Second, some more comments below.

Mar 21 2020, 8:23 PM · Plasma

Mar 17 2020

hpereiradacosta added a comment to D27669: [kstyle] Tools area.

Hi,
finally had some time to double-check into kiconloader, and confirmed that setting the custom palette and reseting is pretty harmless since it does not invalidate the cache. So to me it is fine to leave this part as it is now.
I have added a few more optimization comments below.
Appart from these, I think what's still missing are:

  • an option to disable (both manually and automatically with non-null borders)
  • adressing the remaining comments (such as the unrelated metrics change, which IMO should really go into a separate commit and be justified on its own rather than within this (large) changeset,
Mar 17 2020, 3:20 PM · Plasma
hpereiradacosta accepted D28087: Fix Defaults not being set properly in Breeze window decoration settings for 'Draw a circle around close button' .

Many thanks !

Mar 17 2020, 2:54 AM · Plasma

Mar 14 2020

hpereiradacosta added a comment to D27669: [kstyle] Tools area.

Generating the toolarea colors from the windows colors and making sure the WindowText color still works; is also a solution. But it requires to also change the decoration to use this generated color instead of that of a palette, and an option to go with it. Also: you would still need an option to disable all this, especially when window borders are not "none", or when people want to use their palette decoration color.

Mar 14 2020, 7:25 PM · Plasma
hpereiradacosta added a comment to D27669: [kstyle] Tools area.

For what its worth: what you would need to fix this properly, is a new QIcon::Mode for "in active tool area" that you would map the the right colors when generating a given pixmap from the svg inside kiconloader.
Only then would you be able to cache for a given icon its properly colored pixmap for being inside the tool area; in a regular button; active; disabled, etc ... without the need to resort to different QPalette as done now (namely one palette for active toolarea and one palette for all the rest).

Mar 14 2020, 7:22 PM · Plasma
hpereiradacosta added inline comments to D27669: [kstyle] Tools area.
Mar 14 2020, 7:17 PM · Plasma

Mar 13 2020

hpereiradacosta added a comment to D27892: [RFC] Don't draw shadows on quick tiled or maximized edges.

+1 for the concept and resulting appearance. But, should this maybe be done in KWin instead? That way all window decoration themes would get this fix/change, not just Breeze.

I don't know about the best place but not drawing a border if quick tiled or maximized is also done in breeze.
Also @davidedmundson said to do it on the decorations level.

Mar 13 2020, 8:58 PM · Plasma
hpereiradacosta added inline comments to D27669: [kstyle] Tools area.
Mar 13 2020, 12:20 AM · Plasma
hpereiradacosta added inline comments to D27669: [kstyle] Tools area.
Mar 13 2020, 12:08 AM · Plasma

Mar 11 2020

hpereiradacosta added inline comments to D27669: [kstyle] Tools area.
Mar 11 2020, 3:59 AM · Plasma
hpereiradacosta added a comment to D27669: [kstyle] Tools area.

I noticed there seems to be something wrong in the color logic when

  • active window decoration color is the same as the main window decoration
  • inactive window decoration is different

(essentially the opposite as current breeze theme)
In that case, one would expect the inactive tool area to be colored and the active one not to be. It seems that neither are colored except for the menu bar.
See:


corresponding to an *inactive* window in this modified color scheme (just for illustration purpose)

Mar 11 2020, 3:47 AM · Plasma

Mar 10 2020

hpereiradacosta added inline comments to D27669: [kstyle] Tools area.
Mar 10 2020, 9:38 PM · Plasma
hpereiradacosta added a comment to D27669: [kstyle] Tools area.

A few more comments, but all in all seems to be getting there (beside the options to disable and/or to define the colors based on the QPalette and not the decorations)

Mar 10 2020, 9:31 PM · Plasma
hpereiradacosta added inline comments to D27669: [kstyle] Tools area.
Mar 10 2020, 4:40 PM · Plasma
hpereiradacosta added a comment to D27669: [kstyle] Tools area.
Mar 10 2020, 4:16 PM · Plasma
hpereiradacosta added inline comments to D27669: [kstyle] Tools area.
Mar 10 2020, 1:40 PM · Plasma
hpereiradacosta added a comment to D27669: [kstyle] Tools area.

Some more comments about the code.

Mar 10 2020, 1:24 PM · Plasma
hpereiradacosta added a comment to D27669: [kstyle] Tools area.

For color schemes that have contrasting titlebar and content colors, this patch makes application headers look giant...


It does look good with the proposed default color palette, but it makes many others, including current Breeze, look like an ugly CSD parody :/

Mar 10 2020, 1:08 PM · Plasma
hpereiradacosta added a comment to D27669: [kstyle] Tools area.

Some more comments, mostly code related.
Thanks for addressing all the comments so far !

Mar 10 2020, 3:55 AM · Plasma
hpereiradacosta added a comment to D27669: [kstyle] Tools area.

The toolbar background seems to be broken when there are multiple toolbars in the same row.
See, e.g. ktorrent:


Does not happen for applications that have a single toolbar (most applications these days)
Or multiple toolbars but one per row.
Also happens with kdevelop, or konqueror.
Then also there are problems with colors (for all color schemes that have strong contrast between window decoration and window). But should be discussed once the above is fixed.

Mar 10 2020, 3:41 AM · Plasma

Mar 9 2020

hpereiradacosta added a comment to D27938: 'Classic' and 'Redmond' button icon styles, configurable via Breeze window decoration settings.

In my opinion this should really go into a different decoration. Starting to have "styles inside styles" inside a single decoration is a UI mess.
If you want to turn breeze into a theme engine and not a theme, then so be it, but one must go the full way. Not just essentially duplicating the code to render a different set of visually orthogonal buttons into the engine. The new buttons must have a different entry in the kcm. Not be selectable by an internal option. See how decoration engines like aurorae implements different entries in decoration page as the way it should be.
How is a new user supposed to discover that what she/he sees in the KCM is not the only set of icons she/he can chose ?
Also on the code side, the current patch makes the code maintainability much more complicated.

Mar 9 2020, 3:42 AM · VDG, Breeze, Plasma

Mar 5 2020

hpereiradacosta added inline comments to D27669: [kstyle] Tools area.
Mar 5 2020, 10:15 PM · Plasma

Feb 29 2020

hpereiradacosta added inline comments to D27669: [kstyle] Tools area.
Feb 29 2020, 10:50 PM · Plasma
hpereiradacosta added inline comments to D27669: [kstyle] Tools area.
Feb 29 2020, 9:59 PM · Plasma
hpereiradacosta added inline comments to D27669: [kstyle] Tools area.
Feb 29 2020, 9:46 PM · Plasma
hpereiradacosta updated subscribers of D27669: [kstyle] Tools area.

also adding @davidedmundson in the loop in particular for opinion on the devicepixel ratio business, and in case he can reproduce the crash.

Feb 29 2020, 9:45 PM · Plasma
hpereiradacosta added inline comments to D27669: [kstyle] Tools area.
Feb 29 2020, 9:37 PM · Plasma
hpereiradacosta added inline comments to D27669: [kstyle] Tools area.
Feb 29 2020, 9:29 PM · Plasma
hpereiradacosta added a comment to D27669: [kstyle] Tools area.

right now it crashes all applications (here at least) when you unlock toolbars, move them to a different position, then move them back to top.
Tested with dolphin, okular, etc. All crash.

Cannot reproduce.

Feb 29 2020, 9:24 PM · Plasma

Feb 28 2020

hpereiradacosta added a comment to D27669: [kstyle] Tools area.

Overall, this is good stuff I think (although we should also discuss whether this should be part of breeze or rather a next-gen theme).
Still:
right now it crashes all applications (here at least) when you unlock toolbars, move them to a different position, then move them back to top.
Tested with dolphin, okular, etc. All crash.

Feb 28 2020, 2:11 AM · Plasma

Jan 29 2020

hpereiradacosta added a comment to D26823: Port connections to new syntax.

yeah ok. Just noted this. indeed.

Sorry, I assumed you had seen the thread.
Hope you're ok with it?

Jan 29 2020, 8:20 PM · Plasma
hpereiradacosta accepted D27000: [kstyle] Drop Helper::connection().

thanks !

Jan 29 2020, 6:42 PM · Plasma
hpereiradacosta accepted D26978: [kstyle] Use QX11Info::isCompositingManagerRunning().

Makes sense, thanks !

Jan 29 2020, 5:42 PM · Plasma

Jan 22 2020

hpereiradacosta added a comment to D26823: Port connections to new syntax.
In D26823#599003, @zzag wrote:

This breaks the KDE4 compilation of breeze style (of course !)

Qt 4 build was dropped.

https://mail.kde.org/pipermail/plasma-devel/2020-January/108585.html

Jan 22 2020, 4:36 PM · Plasma
hpereiradacosta added a comment to D26823: Port connections to new syntax.

To compile with kde4:
cmake -DUSE_KDE4=1

Jan 22 2020, 4:28 PM · Plasma
hpereiradacosta added a comment to D26823: Port connections to new syntax.

This breaks the KDE4 compilation of breeze style (of course !)
So if this patch is to stay, then one should officially drop the kde4 support (or make a branch for it).
In that case, much more code can go (a lot of QT Version ifdefs, some CMake magic, etc.)
Otherwise, the kstyle part of this patch at least, should be reverted.

Jan 22 2020, 4:27 PM · Plasma

Jan 21 2020

hpereiradacosta added a comment to D26783: Center only during drawing, not the hit rects.

Thanks for the fix.
On a different topics, I have found a couple more places where the separators look weird. See:


or

In the second screenshot it is barely visible, but that makes it look even more like a bug.
In my opinion we should revisit the heuristics for showing this separators, limiting it to actual sunken, non-transparent scroll areas, as opposed to standalone scrollbars.
opinions ? (about the two screenshots above ? About the solution ?)

Jan 21 2020, 3:33 PM · Plasma

Jan 16 2020

hpereiradacosta added a comment to D26655: show a thin separator between view and scrollbar.

Can the top of the scrollbar go below the column headers instead of next to them?

Like this:

Jan 16 2020, 2:43 PM · Plasma
hpereiradacosta added a comment to D26655: show a thin separator between view and scrollbar.

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.
See:


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

Jan 16 2020, 1:37 PM · Plasma
hpereiradacosta added a comment to D26655: show a thin separator between view and scrollbar.
In D26655#595314, @mart wrote:

I noticed that when hovering on the scrollbar border, while the view area is not focused, the scrollbar handle is light gray (Breeze Dark, dark gray for Breeze) instead of blue. Moving the mouse just slightly more to the right turns the scrollbar handle blue.

That's the difference between thesubControlRect of the handle (that has been moved by one pixel to the right for the line) and the rectangle of the entire scrollbar.

to change this and have the handle always hitting, the subcontrolrects should stay the same and just paint the handle moved by one pixeltough the subcontrol rects not moving.
either is good, probably the latter approach looks a bit dirtier in the code but if you think is better from a design pov, it can be done

Jan 16 2020, 1:32 PM · Plasma

Jan 15 2020

hpereiradacosta accepted D26655: show a thin separator between view and scrollbar.

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 !

Jan 15 2020, 6:45 PM · Plasma
hpereiradacosta added a comment to D26655: show a thin separator between view and scrollbar.

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 ?

Jan 15 2020, 5:36 PM · Plasma
hpereiradacosta added a comment to D26655: show a thin separator between view and scrollbar.

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)

Jan 15 2020, 4:54 PM · Plasma
hpereiradacosta added inline comments to D26655: show a thin separator between view and scrollbar.
Jan 15 2020, 4:23 PM · Plasma
hpereiradacosta added a comment to D26655: show a thin separator between view and scrollbar.

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.

Jan 15 2020, 3:40 PM · Plasma

Jan 14 2020

hpereiradacosta added a comment to D26655: show a thin separator between view and scrollbar.

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.

Jan 14 2020, 9:16 PM · Plasma
hpereiradacosta added a comment to D26655: show a thin separator between view and scrollbar.
In D26655#594280, @mart wrote:

Is this really intended ?
I would at least keep the color blending, making the color stronger only when the scrollbar is actually hovered.

ouch, well spotted, i will fix that :)

Jan 14 2020, 5:13 PM · Plasma
hpereiradacosta added a comment to D26655: show a thin separator between view and scrollbar.

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 item truncation into nothingness
2: the too small handle that few people dislike.

Jan 14 2020, 5:12 PM · Plasma
hpereiradacosta added a comment to D26655: show a thin separator between view and scrollbar.

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.

Jan 14 2020, 4:41 PM · Plasma
hpereiradacosta added a comment to D26655: show a thin separator between view and scrollbar.
Jan 14 2020, 4:32 PM · Plasma
hpereiradacosta added a comment to D26655: show a thin separator between view and scrollbar.

Regarding the thickened scrollbars, I think it makes sense for a few reasons:

  • Acknowledging user feedback: we've had a bunch of complaints about the thin scrollbars.

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).

Jan 14 2020, 4:27 PM · Plasma
hpereiradacosta added a comment to D26655: show a thin separator between view and scrollbar.

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 ?

That problem is caused by overlapping scrollbars, D26530 makes them no longer overlapping thus fixing that problem.

Jan 14 2020, 3:24 PM · Plasma
hpereiradacosta added a comment to D26655: show a thin separator between view and scrollbar.

Hi Notmart,
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 ?

Or is it now fixed ? (I could not see it in your other commit's screenshot. I do see it here in e.g. systemsettings)

Jan 14 2020, 2:45 PM · Plasma

Jan 13 2020

hpereiradacosta accepted D26639: Make checkboxes/radiobuttons use Window Background in windows and View Backround in lists.

Otherwise, it should be Window Background. Doing it that way would preserve the original look in most cases.

This would lead to some regression (I think) for unchecked checkboxes in lists. (because of window background being used).

This doesn't seem to have any visual regressions like that.

Jan 13 2020, 9:04 PM · Plasma
hpereiradacosta added a comment to D26572: Always render checkbox/radiobutton background.

What I meant is that I did not change the color of the checkbox background in this patch. I only made it so that the background would always be rendered.

Jan 13 2020, 7:19 PM · Plasma
hpereiradacosta added a comment to D26572: Always render checkbox/radiobutton background.

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 really revert this part of the change

To be clear, that change hasn't been made.

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.

Jan 13 2020, 6:17 PM · Plasma
hpereiradacosta added a comment to D26572: Always render checkbox/radiobutton background.

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 really revert this part of the change

Jan 13 2020, 5:09 PM · Plasma
hpereiradacosta added a comment to D26572: Always render checkbox/radiobutton background.

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.

Jan 13 2020, 5:02 PM · Plasma
hpereiradacosta accepted D26572: Always render checkbox/radiobutton background.

ship it. No strong feeling.

Jan 13 2020, 3:11 PM · Plasma

Jan 11 2020

hpereiradacosta added a comment to D26572: Always render checkbox/radiobutton background.

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.

Jan 11 2020, 10:40 PM · Plasma

Jan 4 2020

hpereiradacosta committed R31:373954acd434: removed unused variables (authored by hpereiradacosta).
removed unused variables
Jan 4 2020, 4:45 AM
hpereiradacosta committed R31:af4110dec5ca: fixed qt4 compilation (authored by hpereiradacosta).
fixed qt4 compilation
Jan 4 2020, 4:45 AM

Dec 30 2019

hpereiradacosta accepted D26225: Change frameRadius to use pen widths, add frameRadiusForNewPenWidth, add PenWidth::NoPen.
  • Change newPenWidthFrameRadius to frameRadiusNewPenWidth

    This sounds slightly better to me and allows it to show up next to frameRadius when autocompleting.
Dec 30 2019, 12:12 AM · Plasma
hpereiradacosta accepted D26268: Fix QTime function deprecation warnings by switching to QElapsedTimer.
Dec 30 2019, 12:10 AM · Plasma

Dec 29 2019

hpereiradacosta added a comment to D26267: Replace deprecated QWeakPointer::data() with .toStrongRef().data().

-1:
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).

So this patch is not the correct thing to do at all, even if I make the changes @anthonyfieroni suggested. I doubt getting Qt to undeprecate QWeakPointer::data() is an option, so it seems like changing KDecoration would be the correct thing to do. Does that seem correct to you?

Dec 29 2019, 10:42 PM · Plasma
hpereiradacosta added a comment to D26267: Replace deprecated QWeakPointer::data() with .toStrongRef().data().

-1:
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).
Dec 29 2019, 10:12 PM · Plasma

Dec 27 2019

hpereiradacosta added inline comments to D26225: Change frameRadius to use pen widths, add frameRadiusForNewPenWidth, add PenWidth::NoPen.
Dec 27 2019, 10:42 PM · Plasma
hpereiradacosta accepted D26217: Add standard pen widths and replace hardcoded numbers.

looks good. See comment about the symbol pen width change and then ship it.

Dec 27 2019, 10:38 PM · Plasma

Dec 19 2019

hpereiradacosta accepted D26094: Add shadow rendering helper functions.

Looks good and sensible. Very nice consolidation. Thanks !

Dec 19 2019, 3:59 PM · Plasma

Dec 17 2019

hpereiradacosta added a comment to D24281: Add default shortcut to switch to the desktop to the left/right/top/bottom.

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 17 2019, 6:00 PM · KWin

Dec 14 2019

hpereiradacosta added a comment to D19890: Reduce the indicator arrow size for press-and-hold menus in QToolButtons.

As a side remark: did you check how it looks when one selects "text under icon" or "text beside icon" for toolbar button ?

Dec 14 2019, 7:31 PM · Plasma
hpereiradacosta accepted D26001: Fix rubberband selection outline position.

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 14 2019, 7:28 PM · VDG, Breeze, Plasma

Dec 13 2019

hpereiradacosta added a comment to D19890: Reduce the indicator arrow size for press-and-hold menus in QToolButtons.

Ok, so there is an overlap problem, but it's quite rare. It happens when an icon uses 100% of the available space in the bottom right corner (or left with RTL, I think).
Here I changed the stop icon in KDevelop to the icon for Codelite:

.

I could get around this in a number of ways that all have their downsides:
1/ Move the arrow to the very bottom right and make sure it's small enough that no clipping happens

  • This looks bad and makes it harder to see the arrow
Dec 13 2019, 10:31 PM · Plasma

Dec 9 2019

hpereiradacosta added a comment to D25814: [KColorScheme] Add SeparatorColor.

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.

Dec 9 2019, 8:20 PM · Frameworks
hpereiradacosta added a comment to D25814: [KColorScheme] Add SeparatorColor.

Can we clarify what separators we're referring to.

Frames around UI elements and horizontal/vertical lines that separate UI elements.
These are the most common examples, but probably not absolutely everything:

Dec 9 2019, 4:19 PM · Frameworks
hpereiradacosta added a comment to D25814: [KColorScheme] Add SeparatorColor.

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 :) ).

Dec 9 2019, 4:10 PM · Frameworks
hpereiradacosta added a comment to D25814: [KColorScheme] Add SeparatorColor.

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.

Dec 9 2019, 5:36 AM · Frameworks
hpereiradacosta updated subscribers of D25814: [KColorScheme] Add SeparatorColor.

Would also be good to have the opinion of @cfeck on this.

Dec 9 2019, 1:00 AM · Frameworks
hpereiradacosta added a comment to D25814: [KColorScheme] Add SeparatorColor.

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 9 2019, 12:29 AM · Frameworks

Dec 2 2019

hpereiradacosta accepted D25691: [KDecoration] Use QIcon::paint.

Thanks !

Dec 2 2019, 6:52 PM · Plasma

Nov 28 2019

hpereiradacosta accepted D25593: [kdecoration] Use QVariantAnimation instead of QPropertyAnimation.

Thanks !

Nov 28 2019, 7:35 PM · Plasma

Nov 27 2019

hpereiradacosta added a comment to D25543: Split Style & Helper into files by widget type.

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.

Nov 27 2019, 4:12 AM · Plasma
hpereiradacosta added a comment to D25543: Split Style & Helper into files by widget type.

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 ...

Nov 27 2019, 3:53 AM · Plasma
hpereiradacosta added a comment to D25543: Split Style & Helper into files by widget type.

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 27 2019, 3:41 AM · Plasma

Nov 13 2019

hpereiradacosta updated subscribers of T12027: Relicence breeze to LGPL.

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 ?

Nov 13 2019, 7:50 PM · Breeze, Goal: All About the Apps