[KIO/drag and drop] Fix file and folder drag and drop popup menu transparency
AcceptedPublic

Authored by anemeth on Feb 6 2019, 9:58 PM.

Details

Summary

The drag and drop popup menu background was broken everywhere.
This adds the same workaround we used in the desktop icon context menu: D14174 and D15435

BUG: 403440

Test Plan
  1. Set menu transparency in the Breeze widget settings
  2. Drag and drop a file

Expected result:

Diff Detail

Repository
R241 KIO
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 7960
Build 7978: arc lint + arc unit
anemeth created this revision.Feb 6 2019, 9:58 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptFeb 6 2019, 9:58 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
anemeth requested review of this revision.Feb 6 2019, 9:58 PM
anemeth edited the summary of this revision. (Show Details)Feb 6 2019, 10:03 PM
anemeth edited the test plan for this revision. (Show Details)
anemeth added a reviewer: Frameworks.
anemeth added a subscriber: ngraham.
anemeth retitled this revision from Fix file and folder drag and drop popup menu transparency to [KIO/drag and drop] Fix file and folder drag and drop popup menu transparency.Feb 6 2019, 10:18 PM
anemeth added a reviewer: kde-frameworks-devel.
ngraham accepted this revision.Feb 6 2019, 10:19 PM
This revision is now accepted and ready to land.Feb 6 2019, 10:19 PM

I will wait for someone from the frameworks team to review this before landing it.

Sounds good. It's the same thing that's done in other places, so I don't expect there'll be any problem.

Wait, do we need to add this workaround to every popup menu around? What about 3d-party apps?

Can't we fix the actual bug in Qt/breeze/whatever instead?

@anemeth can provide the details, but IIRC we did wind up concluding that we needed this workaround in various places.

Wait, do we need to add this workaround to every popup menu around? What about 3d-party apps?

Can't we fix the actual bug in Qt/breeze/whatever instead?

I believe this is a bug in Breeze, since this works with Kvantum themes without this.
We had this discussion in https://bugs.kde.org/show_bug.cgi?id=395262
I know this is not the best approach, but it's easier to fix it in the most obvious places instead of rewriting Breeze
This fixes the DnD menu on the desktop and in Dolphin as well

pino added a subscriber: pino.Feb 16 2019, 8:54 PM

Wait, do we need to add this workaround to every popup menu around? What about 3d-party apps?

Definitely not an option. IMHO it makes no nsense to add these lines in every single Qt application.

Can't we fix the actual bug in Qt/breeze/whatever instead?

Exactly. Please fix breeze instead.

The workaround isn't required everywhere, just in a small number of places. This is actually the last place that causes user-facing issues with blur that I'm aware of, and prior patches were accepted by Plasma developers, so it would be a shame not to land this last one.

pino added a comment.Feb 17 2019, 9:08 AM

The workaround isn't required everywhere, just in a small number of places.

It still is a workaround, and the fact that the places where this is "needed" are not many still does not justify it IMHO.

prior patches were accepted by Plasma developers, so it would be a shame not to land this last one.

The real shame was to accept such workarounds in other places, instead of fixing breeze (or qt).

Again, -1 for workarounds in important places like KIO, where they will stay for years because people forget about them...

@pino, can you help fix us the root cause then? The reason why we've resorted to these workarounds is because nobody's been able to adequately understand the root cause or implement a risk-free fix.

@pino, can you help fix us the root cause then? The reason why we've resorted to these workarounds is because nobody's been able to adequately understand the root cause or implement a risk-free fix.

Ping. The workaround is known to work and cause no regressions, and I see no reason to torture our users with broken behavior for any longer. I would like to land this for Frameworks 5.56 if we aren't going to be able to work on fixing the root cause anytime soon.

pino added a comment.Feb 23 2019, 3:11 PM

@pino, can you help fix us the root cause then? The reason why we've resorted to these workarounds is because nobody's been able to adequately understand the root cause or implement a risk-free fix.

Ping. The workaround is known to work and cause no regressions, and I see no reason to torture our users with broken behavior for any longer. I would like to land this for Frameworks 5.56 if we aren't going to be able to work on fixing the root cause anytime soon.

The workaround is still a bad workaround, and as such ought to not be in a general-purpose framework such as KIO.

Sorry, I don't have the time to also dig into this issue myself; OTOH, this is not a reason for accepting such broken code.

Also, please do not resort to psychological tricks like "torture our users", as if this is any worse than any behaviour issues there.

Stilla big -1, sorry. You will need a KIO maintainer for their opinion on this "patch".

pino added a comment.Feb 23 2019, 3:14 PM

Let me add also this: this behaviour seems triggered only when using breeze so far; hence, I see two possible explanations:

  1. it is a bug in breeze
  2. it is a bug in Qt, that somehow breeze manages to trigger

In case it's (1), why was this behaviour even added in the first place?

In D18798#418056, @pino wrote:

Also, please do not resort to psychological tricks like "torture our users", as if this is any worse than any behaviour issues there.

There is no psychological trick. This is simply how I see things: we are doing this for the users. Users are experiencing this bug. We have a known, working, low-risk (or even risk-free) patch that fixes the issue. The patch is a workaround rather than a true fix, but this is because nobody has so far figured out how to fix the issue itself without introducing substantially more risk. Given these conditions, I don't see the harm in landing the patch. Everything in life involves trade-offs, and to me, this patch falls into the category of "don't let the perfect be the enemy of the good."

I heartily agree with you on the general principle of avoiding workarounds and fixing issues at their root causes. However so far nobody has been able to do so for this issue. On the other hand, we have a workaround that we already know works and does not cause regressions because we already uses it with success in other places. In this messy world we live in, sometimes workarounds are the lesser evil. The alternative is that this remains broken for users and we continue to get bug reports about it.

If you don't have time to dig into this, do you by any chance know anyone who does?

In D18798#418056, @pino wrote:

Also, please do not resort to psychological tricks like "torture our users", as if this is any worse than any behaviour issues there.

There is no psychological trick. This is simply how I see things: we are doing this for the users. Users are experiencing this bug. We have a known, working, low-risk (or even risk-free) patch that fixes the issue. The patch is a workaround rather than a true fix, but this is because nobody has so far figured out how to fix the issue itself without introducing substantially more risk. Given these conditions, I don't see the harm in landing the patch. Everything in life involves trade-offs, and to me, this patch falls into the category of "don't let the perfect be the enemy of the good."

I heartily agree with you on the general principle of avoiding workarounds and fixing issues at their root causes. However so far nobody has been able to do so for this issue. On the other hand, we have a workaround that we already know works and does not cause regressions because we already uses it with success in other places. In this messy world we live in, sometimes workarounds are the lesser evil. The alternative is that this remains broken for users and we continue to get bug reports about it.

If you don't have time to dig into this, do you by any chance know anyone who does?

@pino also please consider that the same fix in another place was submitted, reviewed, accepted and landed, months ago

pino added a comment.Feb 23 2019, 3:45 PM
In D18798#418056, @pino wrote:

Also, please do not resort to psychological tricks like "torture our users", as if this is any worse than any behaviour issues there.

There is no psychological trick. This is simply how I see things: we are doing this for the users. [...]

KIO is used not only by Plasma users, so it would be very bad to add workaround because of a specific workspace bit.

As I mentioned, please fix breeze instead; if it cannot be fixed, then remove the behaviour that triggered these bugs (this PR and the other workarounds already accepted (sigh)).
I still do not see how it is even a good idea to add code (in breeze) that produces bugs elsewhere, and force "elsewhere" to work around that.

Again, fix breeze, pretty please.

pino added a comment.Feb 23 2019, 3:47 PM

@pino also please consider that the same fix in another place was submitted, reviewed, accepted and landed, months ago

I already said something about this. "we added crap, so let's add even more" is not really a good arguments to use, IMHO.

BTW, here's the bug report tracking the issue in Breeze itself, if anyone wants to take a crack at it: https://bugs.kde.org/show_bug.cgi?id=399680