[platforms/x11] Make X11WindowedOutput::init() private
AbandonedPublic

Authored by zzag on Aug 30 2019, 1:25 PM.

Details

Reviewers
romangg
Group Reviewers
KWin
Summary

This method is meant to be used only by X11WindowedBackend.

Diff Detail

Repository
R108 KWin
Branch
make-init-method-private
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 15853
Build 15871: arc lint + arc unit
zzag created this revision.Aug 30 2019, 1:25 PM
Restricted Application added a project: KWin. · View Herald TranscriptAug 30 2019, 1:25 PM
Restricted Application added a subscriber: kwin. · View Herald Transcript
zzag requested review of this revision.Aug 30 2019, 1:25 PM

So instead of N classes able to access one method they don't need, we have one class which can access all X11WindowedOuptut's private's which it doesn't need.
It gets downcast to everyone outside of X11WindowedBackend, so init() isn't a visible method to any other class outside of the x11/windowed folder.

I don't think I follow what problem we're solving.

zzag added a comment.EditedAug 30 2019, 2:07 PM

So instead of N classes able to access one method they don't need, we have one class which can access all X11WindowedOuptut's private's which it doesn't need.
It gets downcast to everyone outside of X11WindowedBackend, so init() isn't a visible method to any other class outside of the x11/windowed folder.

Such reasoning won't get us anywhere because I can ask "why would N classes have to be able to call init() method which will lead to undefined behavior when kwin is compiled w/o Q_ASSERT?"

It's all about maintaining good public interface. Sometimes it's worth to keep "create" or "init" method public, but in most cases it's discouraged. Even though this code is in "platforms/whatever", it doesn't mean that we should stop writing good code.

Keeping init() public is bad because it pollutes the public interface with implementation details of the class as well it's not safe to call init() many times. In either case encapsulation is "meh," but at least it's less severe with friend class.

In either case encapsulation is "meh," but at least it's less severe with friend class.

It just swaps one bad thing for one equally bad.
Now you have the hypothetical case of some rogue future coder in X11WindowedBackend modifying m_window directly causing a crash.

Anyway, in this case the args and everything can just go in the constructor. You solve the case of being called multiple times and from which class all in one go.

zzag added a comment.Aug 30 2019, 2:47 PM

It just swaps one bad thing for one equally bad.

Yeah, what we have now and what friend keyword is are not far from each other. However, it's better to judge this revision by the end result. Public interface of X11WindowedOutput is leaner with less clutter.

Anyway, in this case the args and everything can just go in the constructor. You solve the case of being called multiple times and from which class all in one go.

Totally agreed, we should embrace RAII rather than fight it, but sometimes it's better to go a bit off road in order to not go insane, for example would you be happy to work with QOpenGLTexture if it had a huge constructor with all possible parameters and no create() method, or if you had to fill in a struct with all parameters (hello, Vulkan!) and pass it to a constructor? :)

As for this patch, I think we can stick with private init() method for now and gradually switch to RAII wherever it's a reasonable thing to do.

romangg requested changes to this revision.Aug 30 2019, 3:29 PM
romangg added a subscriber: romangg.

And I think we can stay keep the public init and switch to something better when we have it. I don't want to check back all the time if a private field has been manipulated by the backend class when working on the code.

If you want to improve the code get rid of the necessity to have the DrmBackend class be friend with DrmOutput. David also gave you a good pointer on what to do here if you really can't stand this public init function: add the arguments to the constructor. But even then the "improvement" wouldn't be worth the time to do it.

This revision now requires changes to proceed.Aug 30 2019, 3:29 PM
zzag abandoned this revision.Sep 23 2019, 3:59 PM