Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FanzyZones no longer working for remote desktop sessions in v0.16 #1821

Closed
GeertvanHorrik opened this issue Apr 1, 2020 · 17 comments · Fixed by #1826
Closed

FanzyZones no longer working for remote desktop sessions in v0.16 #1821

GeertvanHorrik opened this issue Apr 1, 2020 · 17 comments · Fixed by #1826
Labels
Issue-Bug Something isn't working Product-FancyZones Refers to the FancyZones PowerToy Resolution-Fix Committed Fix is checked in, but it might be 3-4 weeks until a release. Severity-Regression This was working in a previous release

Comments

@GeertvanHorrik
Copy link
Contributor

Environment

Windows build number: 10.0.18363.752
PowerToys version: 0.16.0
PowerToy module for which you are reporting the bug (if applicable): FanzyZones

Steps to reproduce

Use Remote Desktop session, FanzyZones is no longer working.

Expected behavior

FanzyZones to work for remote desktop sessions.

Actual behavior

FanzyZones not working for remote desktop sessions.

Screenshots

[not applicable]

@GeertvanHorrik
Copy link
Contributor Author

This issue is related to #1807. Was asked to create a separate ticket.

Here is some info:

I can confirm this as well on my ultra-wide monitor. Happens to my remote desktop sessions. It probably can't find the monitor device anymore. It works fine on the same monitor on the non-RD sessions.

Here is my remote desktop (virtualized) machine zone-settings.json:

{"app-zone-history":[],"devices":[],"custom-zone-sets":[]}

Here is the same monitor, but attached to real hardware:

    "devices": [
        {
            "device-id": "SAM0F99#5&29836fa9&0&UID261_1680_1050_{00000000-0000-0000-0000-000000000000}",
            "active-zoneset": {
                "uuid": "{17837D3A-33B0-4CE0-B8D2-C2CEE228A872}",
                "type": "columns"
            },
            "editor-show-spacing": true,
            "editor-spacing": 16,
            "editor-zone-count": 3
        },
        {
            "device-id": "SAM0F99#5&29836fa9&0&UID261_1920_1080_{00000000-0000-0000-0000-000000000000}",
            "active-zoneset": {
                "uuid": "{17837D3A-33B0-4CE0-B8D2-C2CEE228A872}",
                "type": "columns"
            },
            "editor-show-spacing": true,
            "editor-spacing": 16,
            "editor-zone-count": 3
        },
        {
            "device-id": "SAM0F9C#7&43b830e&0&UID256_5120_1440_{00000000-0000-0000-0000-000000000000}",
            "active-zoneset": {
                "uuid": "{36CC55BE-1BA6-48B1-90AA-8B6E04D2E73E}",
                "type": "priority-grid"
            },
            "editor-show-spacing": false,
            "editor-spacing": 16,
            "editor-zone-count": 3
        }
    ],
    "custom-zone-sets": []

I opened the zones-settings.json in notepad++. Then I open PT and select a new zone. It says the file has changed (so it has rewritten the contents), but the contents remain the same.

Whenever I re-open PT settings for FZ, the settings show the default (which is expected since the file is empty).

I also tried removing the file, and it creates a new one which is empty again.

I am currently debugging PT to see if I can easily fix this.

@enricogior enricogior added Product-FancyZones Refers to the FancyZones PowerToy Issue-Bug Something isn't working Severity-Regression This was working in a previous release labels Apr 1, 2020
@GeertvanHorrik
Copy link
Contributor Author

The temp file created still contains the right monitor info:

{"device-id":"Default_Monitor#1\u00261f0c3c2f\u00260\u0026UID256_5120_1440_{00000000-0000-0000-0000-000000000000}","active-zoneset":{"uuid":"{F5864905-41F2-4A90-8C3F-E2AB556952F1}","type":"priority-grid"},"editor-show-spacing":true,"editor-spacing":16,"editor-zone-count":3}

Results in these parameters being generated:

65537 0_0_5120_1400 5120_1440 "C:\Users\geert\AppData\Local\Temp\xfw4.1" "C:\Users\geert\AppData\Local\Temp\xfw4.2" "C:\Users\geert\AppData\Local\Temp\xfw4.0"

@GeertvanHorrik
Copy link
Contributor Author

Found something interesting.

Input to editor app:

{"device-id":"Default_Monitor#1&1f0c3c2f&0&UID256_5120_1440_{00000000-0000-0000-0000-000000000000}","active-zoneset":{"uuid":"null","type":"blank"},"editor-show-spacing":false,"editor-spacing":0,"editor-zone-count":0}

Output from editor app:

{"device-id":"Default_Monitor#1\u00261f0c3c2f\u00260\u0026UID256_5120_1440_{00000000-0000-0000-0000-000000000000}","active-zoneset":{"uuid":"{BDD36AF5-7509-4EA8-930B-16816D8D66CE}","type":"priority-grid"},"editor-show-spacing":true,"editor-spacing":16,"editor-zone-count":3}

Notice how the device id has changed formatting. This causes this method not to recognize the device:

image

@GeertvanHorrik
Copy link
Contributor Author

I am fairly certain this is caused by the json serialization:

image

@GeertvanHorrik
Copy link
Contributor Author

Caused by this commit:

8156279

@enricogior
Copy link
Contributor

@GeertvanHorrik
thanks for debugging it!

@GeertvanHorrik
Copy link
Contributor Author

GeertvanHorrik commented Apr 1, 2020

The first fix in is in LayoutModel::Apply of the editor app:

            // Note: The 'UnsafeRelaxedJsonEscaping` makes sure that monitor id's that contain '&' are not encoded
            JsonSerializerOptions options = new JsonSerializerOptions
            {
                PropertyNamingPolicy = new DashCaseNamingPolicy(),
                Encoder = JavaScriptEncoder.UnsafeRelaxedJsonEscaping,
            };

Another bug (related to this) is in JSONHelper::isValidDeviceId. The input value:

Default_Monitor#1&1f0c3c2f&0&UID256_5120_1440_{00000000-0000-0000-0000-000000000000}

This method splits by _ and force checks whether it has 4 elements. But... because the name is Default_Monitor, it will break because it results in a set of 5.

The solution here is to split the first element until the #.

@enricogior
Copy link
Contributor

Yes isValidDeviceId needs to be fixed

@GeertvanHorrik
Copy link
Contributor Author

I am fixing both in a PR if you are open to it.

@enricogior
Copy link
Contributor

I think we only need to fix isValidDeviceId but let me verify it.

@enricogior
Copy link
Contributor

The fact that the device id is encoded shouldn't be a problem since it gets correctly decoded when parsed in the C++ code, so let's fix only isValidDeviceId, and then we can verify for sure if it's enough. Thanks!

@GeertvanHorrik
Copy link
Contributor Author

Ok. I will push a fix for isValidDeviceId only.

@GeertvanHorrik
Copy link
Contributor Author

Note that I only tested the fix combined with the encoding fix in the editor. Then it correctly works on my RDP session.

@enricogior
Copy link
Contributor

Just to make sure: did you also test it without the encoding fix and it didn't work?

@GeertvanHorrik
Copy link
Contributor Author

Just to make sure: did you also test it without the encoding fix and it didn't work?

No, I didn't test with encoding disabled.

enricogior pushed a commit that referenced this issue Apr 2, 2020
…JSONHelpers.isValidDeviceId (#1826)

* #1821 Support device names with underscores (such as 'Default_Monitor') in JSONHelpers.isValidDeviceId

* Make support for '#' in device names optional

* Add more unit tests for isValidDeviceId
@enricogior enricogior added the Resolution-Fix Committed Fix is checked in, but it might be 3-4 weeks until a release. label Apr 2, 2020
@enricogior
Copy link
Contributor

Reopening until we release 0.16.1

@enricogior
Copy link
Contributor

Fix available in 0.16.1 https://github.com/microsoft/PowerToys/releases
Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue-Bug Something isn't working Product-FancyZones Refers to the FancyZones PowerToy Resolution-Fix Committed Fix is checked in, but it might be 3-4 weeks until a release. Severity-Regression This was working in a previous release
Projects
None yet
2 participants