but I wanted to illustrate some of the unintended effects of your patch.

1920x1080 + /smart-sizing:3840x2160: bounds the window to 1920x1080, but xfreerdp is showing a 3840x2160 window, so it is cropped. (works without patch)

it will actually set a resolution of 7680x4320?

Any review? I believe this is a bug fix and should not affect users without scaling so I'd like to merge in a few days if no one else is reviewing...

Ping?

Add comment about preserving error messages for the user.

Include more disconnection cases than the ones from the vnc library.

P.S. I'll add that I do prefer my version of the patch since it doesn't require multiplying and then dividing... Not particularly comfortable with that when scaling integers by a floating point factor....

What is holding back this review now? I've recently hit https://bugs.kde.org/show_bug.cgi?id=417310 (ok, 3 months ago... not that recent...)

That's possible. This appears to be added (back?) with https://code.qt.io/cgit/qt/qtbase.git/commit/?id=341c8b9cd08 last May which is newer than the 5.12 branch point it seems. My kalarm --help (as well as most other KDE/Qt programs) shows (a variant of),

Ping?

Yeah I was also looking for a test and didn't find any...

Grammar & format.

Change comment style.

P.S. is 404 really the number of this repo?.... I seriously thought there was an error when searching for kmine here showed me R404: KMines .....

Not necessarily, QStringList::at(0) could return an empty QString instance if the list is empty.

Didn't know this can happen and lgtm.

Thanks, did you test?

It feels like the API could have hidden this completely and do all the dirty work when setting the style name... Anyway, the change looks right given the description of the problem and if this is what we have to deal with so be it........

This is a qt bug that I can't reproduce and the change looks safe so LGTM....

In this case it's really an implementation detail, meant to unburden the main class which is becoming unwieldy, and make more isolated changes that don't require rebuilding the majority of the plugin each time you make a small change.

Implementation detail: the patch introduces a d pointer to a private subclass,

But I don't see how could do either for startTimer()?

Style::drawControl() is const, so one also has to move m_timer, m_progressBarAnimateTimer and m_progressBarAnimateFps to a private class, which in addition would have to have a q pointer and a startParentTimer() "proxy" so we can call Style::startTimer() from a const member function.

LGTM.

Errors like: Error at qt5/CMakeLists.txt:34 (find_package):

Hmmm, option variable usually *are* stored in the cache (so that their setting persists between CMake invocations), but they indeed do have to be set to a default variable. It's possible my change didn't do that properly.

Also, what's the error mentioned in edebb86de2e2818f831cd2ac1967f12b83645b33 ? Does the find_package call errors even with QUIET specified? Conditionally running it should't cause much issue (qt4 is not the only one that's looking at kde4 settings but it probably doesn't matter too much at this point) the default value of enable_kde4/kf5 shouldn't need to depend on qt4/qt5 settings. That should remove cmake_dependent_option.

Hmm, why am I only hearing that now

automatic detection can still work.

It seems that there was an attempt to only do this if ENABLE_QT5 is not explicitly defined, but that test is flawed. There is no way to tell the difference between loading from a previous cache and the command line as -D is literally just setting a cache variable before it starts processing.

changed the shadow size from 6px to 30px

Only very minor comments.

GTK config storing

The change LGTM. There's another copy in gtk2/common which should ideally be kept in sync (to not make it even harder to merge later) but that shouldn't cause any immediate problems since I don't think that version ever save config file.

I'm not familiar with what's the oldest Qt version that is still shipped by supported distributions but if

Assuming the coordinate calculations are correct (which I can never get right without seeing the result) this LGTM.

I would, but this is new in Qt5.5

Looks correct.

Is this the new review platform?

In D3602#70700, @colomar wrote:

While this proposed behavior might indeed not be expected by everyone, this is not really a problem: Those who would not expect the wrap-around behavior would not expect anything to happen at all, and therefore not even press the shortcut if they are at the left-/rightmost window. If they do press it anyway (maybe by accident), they might be surprised at first, but it will be easy to them to get back to where they were before, and then they can still decide whether or not they want to use the shortcut in such situations in the future.