Overhaul AbstractOutput
ClosedPublic

Authored by zzag on Jun 17 2019, 10:59 AM.

Details

Summary

There are several issues with code of AbstractOutput class:

(a) Some methods are documented, and some are not. In general, we tend

to document all public methods in KWin core. It looks like a very
minor issue, but there are methods that have very ambiguous return
value. One such method is geometry(). It's not obvious whether the
returned geometry is in device independent pixels or not;

(b) There's a mix of methods defined in the cpp file and in the header.

This is not very good because reading such code becomes a bit harder
if you don't use any fancy IDE;

(c) Missing Q_DISABLE_COPY, etc.

This change addresses these issues, so the code is a bit more readable
and easier to work with.

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.
zzag created this revision.Jun 17 2019, 10:59 AM
Restricted Application added a project: KWin. · View Herald TranscriptJun 17 2019, 10:59 AM
Restricted Application added a subscriber: kwin. · View Herald Transcript
zzag requested review of this revision.Jun 17 2019, 10:59 AM
davidedmundson added inline comments.
abstract_output.h
109

Either the comment before was wrong, or the new comment here is wrong.

mHz is 10^6 times per second

ms is 10^-3 seconds

cfeck added a subscriber: cfeck.Jun 17 2019, 11:16 AM

mHz != MHz

broulik added inline comments.
abstract_output.h
109

mHz is 10^-3 times per second, no?
You're thinking MHz here.

davidedmundson accepted this revision.Jun 17 2019, 11:19 AM
davidedmundson added inline comments.
abstract_output.h
109

I was.

In my country we have clearer units!

This revision is now accepted and ready to land.Jun 17 2019, 11:19 AM
abstract_output.h
109

I got mixed up in my explanation, but I think my original comment was right.

if something is 1/ms that's once per millisecond. i.e thousand times per seconds
which makes it 1kHz
not 1mHz

zzag added inline comments.Jun 17 2019, 11:36 AM
abstract_output.h
109

Current refresh rate in 1/ms.

Given this description it should be kHz, but 1/ms is incorrect assumption. The calculated refresh rate is in mHz (millihertz), see https://gitlab.freedesktop.org/wayland/weston/blob/master/libweston/backend-drm/drm.c#L4769-4786.

This revision was automatically updated to reflect the committed changes.