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