Remove Client fullscreen hack
ClosedPublic

Authored by romangg on Jan 10 2019, 3:18 PM.

Details

Summary

This hack seems to be deactivated by default nowadays and I did not find an
option in the KCMs to activate it. Git blame shows some last commits from
around 2012 to the functions, but they only deal with code style.

Also even if it would be enabled, in light of Wayland we don't want some
roque XWayland client go bonkers via this hack.

This patch removes the code path in order to decrease complexity and ease
future changes to the fullscreen handling overall.

Test Plan

Manually and autotests pass.

Diff Detail

Repository
R108 KWin
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
romangg created this revision.Jan 10 2019, 3:18 PM
Restricted Application added a project: KWin. · View Herald TranscriptJan 10 2019, 3:18 PM
Restricted Application added a subscriber: kwin. · View Herald Transcript
romangg requested review of this revision.Jan 10 2019, 3:18 PM
romangg planned changes to this revision.Jan 10 2019, 4:24 PM

I think I can remove the stuff in Options as well without worries when I remove the corresponding kcfg key.

For safety I suggest to push after 5.15 has been branched off. That gives us more time to test that nobody still has that key around. Though it's unlikely.

For safety I suggest to push after 5.15 has been branched off. That gives us more time to test that nobody still has that key around. Though it's unlikely.

Thanks for the feedback. If you also think this hack can be removed, then I'm not so worried about removing used functionality.

Just a question: what would be the outcome of somebody still having the key around (in the KWin config file)? I assumed it would just mean that the key would be ignored when reading out the file, but nothing more.

romangg updated this revision to Diff 49222.Jan 11 2019, 10:13 AM
  • Remove option entry completely
zzag added a subscriber: zzag.Jan 11 2019, 10:27 AM

Please keep in mind that Options is exposed to scripting, though I doubt any script checks or sets legacyFullscreenSupport.

client.h
523

Unrelated change.

For safety I suggest to push after 5.15 has been branched off. That gives us more time to test that nobody still has that key around. Though it's unlikely.

Thanks for the feedback. If you also think this hack can be removed, then I'm not so worried about removing used functionality.

I was worried when I saw the change, to be honest. So I did a check that it's not set anywhere. Given that this option is old and all configuration broke with the switch to Qt 5 this is probably safe to remove. I doubt that anybody relies on it as it's impossible that configuration options did the migration from KDE 3 to Plasma 5.

Just a question: what would be the outcome of somebody still having the key around (in the KWin config file)? I assumed it would just mean that the key would be ignored when reading out the file, but nothing more.

Yes.

zzag added a comment.Feb 8 2019, 2:44 PM

Looks good to me, though could you please rebase it on D18128?

romangg updated this revision to Diff 59810.Jun 14 2019, 6:01 PM

Rebase on master.

zzag added a comment.EditedSun, Jul 7, 9:19 PM

Hmm, boolean trap in bool Client::isFullScreenable(bool fullscreenHack) const can be removed.

Coding style patches will make life harder for whoever wants to git blame changes. Can you rebase this change on master? i.e. break dependency between this patch and D18153(as well D18132).

romangg updated this revision to Diff 61328.Mon, Jul 8, 10:59 AM

Rebase on master.

romangg updated this revision to Diff 61329.Mon, Jul 8, 11:12 AM

Remove superfluous boolean trap. Thanks @zzag for spotting it.

zzag added a comment.Mon, Jul 8, 5:09 PM

The patch looks okay, but it still depends on the coding style patches.

It makes more sense to delete something and then fix coding style, rather than fix coding style of something and then delete it.

zzag accepted this revision.EditedMon, Jul 8, 6:06 PM

Argh, never mind. Those patches are already in master.

Please do cleanups before fixing coding style in the future, so git history looks sane.

This revision is now accepted and ready to land.Mon, Jul 8, 6:06 PM
In D18157#492476, @zzag wrote:

Argh, never mind. Those patches are already in master.

Please do cleanups before fixing coding style in the future, so git history looks sane.

I don't agree that then git history in general looks more sane nor that it's an important goal to optimize for. It has very low priority in comparison to making the code work without bugs. On the other side doing the coding style fix before refactoring makes the refactoring simpler. Since regressions are more likely to be introduced on refactoring than on code style changes the latter should be done first to make regressions less likely on refactor.

romangg updated this revision to Diff 61392.Tue, Jul 9, 8:40 AM

Rebase on master.

This revision was automatically updated to reflect the committed changes.