it looks sane to me, but otoh i don't have a hidpi display to test so not really an expert in this field.

Would you mind writing a long-ish comment in the commit (use arc diff --edit --verbatim to reupload the new text to phabricator) for the next person that tries to read the code?

I could test with hidpi screens, but I need to understand what was the problem and how is it fixed.

BTW you can test even without a high DPI screen just by using KScreen's scaling or running QT_SCALE_FACTOR=<some real between 1.0 and 2.0> <executable name/path> It will simply be bigger, but any graphical glitches can still be seen.

I'm going to rebase it on D21096. Please, review that change first.

I don't know if it's what you where trying to fix or not, but when running with

QT_SCALE_FACTOR=2 krdc

i get this weird behaviour where the screen goes white while repainting.

See https://i.imgur.com/omnnTJD.gif

I don't get that running krdc without QT_SCALE_FACTOR

This change prevents highdpi scaling of the remote desktop.
It's double-scaled on your gif and looks pixelated.

This adds new warnings:

../vnc/vncclientthread.h: In constructor ‘VncClientThread::VncClientThread(QObject*)’:
../vnc/vncclientthread.h:191:19: warning: ‘VncClientThread::m_stopped’ will be initialized after [-Wreorder]
     volatile bool m_stopped;
                   ^~~~~~~~~
../vnc/vncclientthread.h:184:11: warning:   ‘qreal VncClientThread::m_devicePixelRatio’ [-Wreorder]
     qreal m_devicePixelRatio;
           ^~~~~~~~~~~~~~~~~~
../vnc/vncclientthread.cpp:323:1: warning:   when initialized here [-Wreorder]
 VncClientThread::VncClientThread(QObject *parent)

member declaration order in .h-file is mismatches initialization order in constructor.

The patch compiles and works.
I've built master branch and with this patch, and run both with QT_SCALE_FACTOR=2. During connection I chose maximum quality option (local network). The results look identical to me:
master on the left, patched D20790 on the right

How should it look like?

Have you installed libkrdc_vncplugin.so into the system?
KRDC doesn't search for plugins in the build tree.

In D20790#481278, @volkov wrote:

Have you installed libkrdc_vncplugin.so into the system?
KRDC doesn't search for plugins in the build tree.

No, I was running from the build tree. :)

Now I see, local window is scaled, remote contents are not, is that the intention of this patch?

Yes, remote contents should not be scaled.

I have no questions to code, but.. This is a behavioral change and I think we need a mantainer's opinion here. Or at least somebody else's opinion.

My first thought was that if you have a 4K monitor and setup a scaling (150% or 200%) - you expect everything to become bigger, window elements, controls, fonts, .. window contents too. With this patch krdc window itself will look OK on your 4K screen on your 200% scaling, but remote contents will look tiny, twice smaller than you would expect. Is that OK? I'll try to make a screenshot later today or tomorrow..

This change allows to show remote contents as is.
For example, remote desktop may be started with QT_SCREEN_SCALE_FACTORS=2 and it will look good on highdpi monitor.

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

The following revised patch from what I mentioned in the bug report allows krdc to continue working on my system.

diff --git a/rdp/rdpview.cpp b/rdp/rdpview.cpp
index 4adc4cc..6c8df98 100644
--- a/rdp/rdpview.cpp
+++ b/rdp/rdpview.cpp
@@ -180,10 +180,11 @@ bool RdpView::start()
     m_containerWidget->setFixedWidth(width);
     m_containerWidget->setFixedHeight(height);
     setMaximumSize(width, height);
+    qreal pixelRatio = m_container->devicePixelRatio();
     if (versionOutput.contains(QLatin1String(" 1.0"))) {
         qCDebug(KRDC) << "Use FreeRDP 1.0 compatible arguments";

-        arguments << QStringLiteral("-g") << QString::number(width) + QLatin1Char('x') + QString::number(height);
+        arguments << QStringLiteral("-g") << QString::number(width * pixelRatio) + QLatin1Char('x') + QString::number(height * pixelRatio);

         arguments << QStringLiteral("-k") << keymapToXfreerdp(m_hostPreferences->keyboardLayout());

@@ -276,8 +277,8 @@ bool RdpView::start()
         qCDebug(KRDC) << "Use FreeRDP 1.1+ compatible arguments";

         arguments << QStringLiteral("-decorations");
-        arguments << QStringLiteral("/w:") + QString::number(width);
-        arguments << QStringLiteral("/h:") + QString::number(height);
+        arguments << QStringLiteral("/w:") + QString::number(width * pixelRatio);
+        arguments << QStringLiteral("/h:") + QString::number(height * pixelRatio);

         arguments << QStringLiteral("/kbd:") + keymapToXfreerdp(m_hostPreferences->keyboardLayout());

which is basically the same as the RDP part (slightly smaller change).

I don't know what was changed for my when upgrading to 19.12 (AFAICT there was no Qt5 upgrade) that cause the behavior change for me and I also can't say what's the correct fix for VNC. However,

1. KRDC is currently unusable as is for rdp and AFAICT this is not a behavior change on RDP when krdc remains usable. i.e. the rdp part of the change should not affect anyone that is affected by the issue already.
2. While scaling the remote side is nice, it should ideally be done in a way that still allow the remote side to take advantage of the higher resolution. A local scaling option would be nice to have if possible if the protocol or the remote machine cannot be configured to scale appropriately. However, that should be an additional feature that should not hold back a bug fix.

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

Also, I found this review request when searching if krdc has any active review here in order to determine if I should send mine to here or gitlab...

@yuyichao, your patch changes requested resolution,
If I choose screen size to be 800x600, it can be surprising to see the remote session starting with 1600x1200.

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

I can also merge only the rdp part which is much less usable without the fix.

So, with this patch if I have my monitor set to 3840x2160 and my scaling set to 2.0. If I select 3840x2160 in KRDC, it will actually set a resolution of 7680x4320?

That seems very unexpected and a change in behavior. Especially if "Current Screen Resolution" is selected.

it will actually set a resolution of 7680x4320?

No

I tested this and it did this:

I looked and realized I had set /smart-sizing:3840x2160

It seems like that is a much better way to handle this in RDP

(it did work correctly at 3840x2160 with and without the patch)

I did some further RDP testing with local scaling set to 2.0

Current KRDC Size: fixed with this
Current Screen Resolution: now behaves like "Current KRDC Size" (Misbehaved in a different way without patch)
1920x1080: works
3180x2160: works
1920x1080 + /smart-sizing:3840x2160: bounds the window to 1920x1080, but xfreerdp is showing a 3840x2160 window, so it is cropped. (works without patch)

Any thoughts about setting /scale-desktop and /scale-device based on local scaling? Maybe only valid for "Current Screen Resolution"

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

I don't think setting it in a way that krdc doesn't expect is ever supported. You'll need to update that setting (the additional option you specify manually)

Any thoughts about setting /scale-desktop and /scale-device based on local scaling? Maybe only valid for "Current Screen Resolution"
I looked and realized I had set /smart-sizing:3840x2160
It seems like that is a much better way to handle this in RDP

As I said before.

While scaling the remote side is nice, it should ideally be done in a way that still allow the remote side to take advantage of the higher resolution. A local scaling option would be nice to have if possible if the protocol or the remote machine cannot be configured to scale appropriately. However, that should be an additional feature that should not hold back a bug fix.

While scaling the remote side is nice, it should ideally be done in a way that still allow the remote side to take advantage of the higher resolution. A local scaling option would be nice to have if possible if the protocol or the remote machine cannot be configured to scale appropriately. However, that should be an additional feature that should not hold back a bug fix.

This assumes unlimited bandwidth. I have much better performance with 1920x1080 resolution and just scale it up locally. I could run at 4k with remote scaling, but it really impacts the responsiveness. Setting the extra option is outside the scope of KRDCs current resolution settings, but I wanted to illustrate some of the unintended effects of your patch.

You could probably fix the RDP current resolution setting like this:

QScreen *screen = rdpUi.kcfg_Width->screen();
QSize size = screen->size() * screen->devicePixelRatio();

rdpUi.kcfg_Width->setValue(size.width());
rdpUi.kcfg_Height->setValue(size.height());

We probably need to expand the rdp settings...

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

...... (It's not my patch but anyway......)

You could probably fix the RDP current resolution setting like this:

QScreen *screen = rdpUi.kcfg_Width->screen();
QSize size = screen->size() * screen->devicePixelRatio();

rdpUi.kcfg_Width->setValue(size.width());
rdpUi.kcfg_Height->setValue(size.height());

Oh, yes the handling of using screen resolution might need to be updated. @volkov

We probably need to expand the rdp settings...

You mean adding scaling options? I agree, but shoudn't let that hold back bug fixes. You are very welcome to add such an option.

RDP changes look fine except for the resize code on creation and tab change.

If everyone is really happy about that, please remember to commit it to the release/20.08 branch first and then mege to master.