Differential D24529

[XembedSNIProxy] Check size in each update()
ClosedPublic
Actions

Authored by kmaterka on Oct 10 2019, 9:22 AM.

Details

Reviewers
davidedmundson
broulik
Group Reviewers
Plasma
Plasma: Workspaces
Commits
R120:acf91005b60c: [XembedSNIProxy] Check size in each update()
Summary

If needed, perform resize in each update().
Client window may be too big, it must be resized to a certain reasonable size. Sometimes resizing in the constructor is not enough because it is too early and the client window is not fully initialized. Even if client window is resized, on first update it reverts back to original size (or just ignores the first request - some kind of a race condition).

In addition, I applied some rules from: https://community.kde.org/Policies/Kdelibs_Coding_Style

BUG: 358240
FIXED-IN: 5.17.1

Test Plan

I tested it using keepass2, now it shows better results (still not perfect). Window is resized to 32x32, but icon is only 24x24 it top-left. Better than 273x273 with tiny dot in the corner. This is a workaround for unsolvable bug in Mono - AFAIK Windows Forms are not longer maintained so it won't be fixed.

I tested this change with several other apps for regressions, no issues:
pidgin
liferea
hexchat
tuxguitar
xchat
keepassx
stardict
and other I don't remember now. No regressions.

Diff Detail

Repository
R120 Plasma Workspace
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
kmaterka created this revision.Oct 10 2019, 9:22 AM
Restricted Application added a project: Plasma. · View Herald TranscriptOct 10 2019, 9:22 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
kmaterka requested review of this revision.Oct 10 2019, 9:22 AM
broulik added a subscriber: broulik.Oct 10 2019, 9:28 AM

Thanks for your patch! Generally we prefer coding style changes to be done separately as to not pollute the change with seemingly unrelated changes.

kmaterka added a comment.Oct 10 2019, 10:44 AM
In D24529#544568, @broulik wrote:

Thanks for your patch! Generally we prefer coding style changes to be done separately as to not pollute the change with seemingly unrelated changes.

Sure, you are right. I will prepare new patch with related changes only and separate with coding style changes.

kmaterka updated this revision to Diff 67596.Oct 10 2019, 10:56 AM
kmaterka added a reviewer: broulik.

Removed code style changes

davidedmundson accepted this revision.Oct 10 2019, 11:57 AM
davidedmundson added inline comments.
xembed-sni-proxy/sniproxy.cpp
217

It's a bit weird to have a method called "getSomething" that changes the state of things

This revision is now accepted and ready to land.Oct 10 2019, 11:57 AM
kmaterka added inline comments.Oct 10 2019, 12:07 PM
xembed-sni-proxy/sniproxy.cpp
217

True, get* should not have side effect. I will change it.

kmaterka updated this revision to Diff 67601.Oct 10 2019, 12:16 PM

getClientWindowSize -> calculateClientWindowSize

ngraham edited the summary of this revision. (Show Details)Oct 10 2019, 2:31 PM
Closed by commit R120:acf91005b60c: [XembedSNIProxy] Check size in each update() (authored by kmaterka, committed by ngraham). · Explain WhyOct 10 2019, 2:32 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 67611

View Options

xembed-sni-proxy/sniproxy.h

Loading...
View Options

xembed-sni-proxy/sniproxy.cpp

Loading...