D10040: Add serial number and EISA ID to OutputDevice interface

[David Edmundson]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit R127:f3378c6808e4: Add serial number and EISA ID to 
OutputDevice interface (authored by dvratil, committed by davidedmundson).

REPOSITORY
  R127 KWayland

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10040?vs=38772&id=39322

REVISION DETAIL
  https://phabricator.kde.org/D10040

AFFECTED FILES
  autotests/client/test_wayland_outputdevice.cpp
  src/client/outputdevice.cpp
  src/client/outputdevice.h
  src/client/protocols/outputdevice.xml
  src/server/outputdevice_interface.cpp
  src/server/outputdevice_interface.h

To: davidedmundson, graesslin, sebas, #kwin, dvratil, romangg
Cc: romangg, kde-frameworks-devel, davidedmundson, plasma-devel, ragreen, 
Pitel, schernikov, michaelh, ZrenBot, ngraham, bruns, alexeymin, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D10040: Add serial number and EISA ID to OutputDevice interface

[Roman Gilg]

romangg accepted this revision.

REPOSITORY
  R127 KWayland

REVISION DETAIL
  https://phabricator.kde.org/D10040

To: davidedmundson, graesslin, sebas, #kwin, dvratil, romangg
Cc: romangg, kde-frameworks-devel, davidedmundson, plasma-devel, ragreen, 
Pitel, schernikov, michaelh, ZrenBot, ngraham, bruns, alexeymin, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D10040: Add serial number and EISA ID to OutputDevice interface

[David Edmundson]

davidedmundson added a comment.


  Bug explained with a fix  here D14505 

REPOSITORY
  R127 KWayland

REVISION DETAIL
  https://phabricator.kde.org/D10040

To: davidedmundson, graesslin, sebas, #kwin, dvratil
Cc: romangg, kde-frameworks-devel, davidedmundson, plasma-devel, ragreen, 
Pitel, schernikov, michaelh, ZrenBot, ngraham, bruns, alexeymin, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D10040: Add serial number and EISA ID to OutputDevice interface

[Roman Gilg]

romangg added a comment.


  In D10040#300588 , @davidedmundson 
wrote:
  
  > I just removed handling of dynamically updating eisa/serialNumber it 
doesn't seem to be something that makes sense for it to change at runtime.
  >
  > Also I don't want to copy the current setEdid pattern, which is broken ATM. 
Whenever any new client connects it broadcasts a change to every connected 
client...
  >  I need to follow that up in another patch, possibly by making it static 
like these two.
  
  
  Can you explain both points a bit more? Maybe when updating the Summary.

REPOSITORY
  R127 KWayland

REVISION DETAIL
  https://phabricator.kde.org/D10040

To: davidedmundson, graesslin, sebas, #kwin, dvratil
Cc: romangg, kde-frameworks-devel, davidedmundson, plasma-devel, ragreen, 
Pitel, schernikov, michaelh, ZrenBot, ngraham, bruns, alexeymin, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D10040: Add serial number and EISA ID to OutputDevice interface

[David Edmundson]

davidedmundson added a comment.


  I just removed handling of dynamically updating eisa/serialNumber it doesn't 
seem to be something that makes sense for it to change at runtime.
  
  Also I don't want to copy the current setEdid pattern, which is broken ATM. 
Whenever any new client connects it broadcasts a change to every connected 
client...
  I need to follow that up in another patch, possibly by making it static like 
these two.

REPOSITORY
  R127 KWayland

REVISION DETAIL
  https://phabricator.kde.org/D10040

To: davidedmundson, graesslin, sebas, #kwin, dvratil
Cc: romangg, kde-frameworks-devel, davidedmundson, plasma-devel, ragreen, 
Pitel, schernikov, michaelh, ZrenBot, ngraham, bruns, alexeymin, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D10040: Add serial number and EISA ID to OutputDevice interface

[David Edmundson]

davidedmundson updated this revision to Diff 38772.
davidedmundson added a comment.


  Remove unrelated changes

REPOSITORY
  R127 KWayland

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10040?vs=38734&id=38772

BRANCH
  output_changes

REVISION DETAIL
  https://phabricator.kde.org/D10040

AFFECTED FILES
  autotests/client/test_wayland_outputdevice.cpp
  src/client/outputdevice.cpp
  src/client/outputdevice.h
  src/client/protocols/outputdevice.xml
  src/server/outputdevice_interface.cpp
  src/server/outputdevice_interface.h

To: davidedmundson, graesslin, sebas, #kwin, dvratil
Cc: romangg, kde-frameworks-devel, davidedmundson, plasma-devel, ragreen, 
Pitel, schernikov, michaelh, ZrenBot, ngraham, bruns, alexeymin, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D10040: Add serial number and EISA ID to OutputDevice interface

[Roman Gilg]

romangg added inline comments.

INLINE COMMENTS

> outputdevice.cpp:161
> +  int32_t subPixel, const char *make, const char 
> *model,
> +  int32_t transform)
>  {

Unrelated change.

> outputdevice_interface.cpp:153
> +connect(this, &OutputDeviceInterface::serialNumberChanged,   this, 
> [this, d] { d->updateGeometry(); });
> +connect(this, &OutputDeviceInterface::eisaIdChanged, this, 
> [this, d] { d->updateGeometry(); });
>  connect(this, &OutputDeviceInterface::scaleChanged,  this, 
> [this, d] { d->updateScale(); });

needs fix

> outputdevice_interface.cpp:410
> +wl_resource_post_event(resource,
> +ORG_KDE_KWIN_OUTPUTDEVICE_GEOMETRY,
>  globalPosition.x(),

Why change? Unrelated.

> outputdevice_interface.h:50
> +//KF6 TODO - This class sends absolute garbage over the wire constantly.
> +//sendDone needs to be explicit, anything related to the geometry event 
> needs to be in a single method
> +

What do you mean with "needs to be explicit"?

REPOSITORY
  R127 KWayland

REVISION DETAIL
  https://phabricator.kde.org/D10040

To: davidedmundson, graesslin, sebas, #kwin, dvratil
Cc: romangg, kde-frameworks-devel, davidedmundson, plasma-devel, ragreen, 
Pitel, schernikov, michaelh, ZrenBot, ngraham, bruns, alexeymin, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D10040: Add serial number and EISA ID to OutputDevice interface

[David Edmundson]

davidedmundson updated this revision to Diff 38734.
davidedmundson added a comment.


  exciting whitespace change

REPOSITORY
  R127 KWayland

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10040?vs=38733&id=38734

BRANCH
  output_changes

REVISION DETAIL
  https://phabricator.kde.org/D10040

AFFECTED FILES
  autotests/client/test_wayland_outputdevice.cpp
  src/client/outputdevice.cpp
  src/client/outputdevice.h
  src/client/protocols/outputdevice.xml
  src/server/outputdevice_interface.cpp
  src/server/outputdevice_interface.h

To: davidedmundson, graesslin, sebas, #kwin, dvratil
Cc: kde-frameworks-devel, davidedmundson, plasma-devel, ragreen, Pitel, 
schernikov, michaelh, ZrenBot, ngraham, bruns, alexeymin, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D10040: Add serial number and EISA ID to OutputDevice interface

[David Edmundson]

davidedmundson updated this revision to Diff 38733.
davidedmundson added a comment.


  Instead of modifying the geometry event use two new events to remain
  fully compatiable.

REPOSITORY
  R127 KWayland

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10040?vs=25806&id=38733

BRANCH
  output_changes

REVISION DETAIL
  https://phabricator.kde.org/D10040

AFFECTED FILES
  autotests/client/test_wayland_outputdevice.cpp
  src/client/outputdevice.cpp
  src/client/outputdevice.h
  src/client/protocols/outputdevice.xml
  src/server/outputdevice_interface.cpp
  src/server/outputdevice_interface.h

To: davidedmundson, graesslin, sebas, #kwin, dvratil
Cc: kde-frameworks-devel, davidedmundson, plasma-devel, ragreen, Pitel, 
schernikov, michaelh, ZrenBot, ngraham, bruns, alexeymin, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D10040: Add serial number and EISA ID to OutputDevice interface

[David Edmundson]

davidedmundson commandeered this revision.
davidedmundson added a reviewer: dvratil.

REPOSITORY
  R127 KWayland

REVISION DETAIL
  https://phabricator.kde.org/D10040

To: davidedmundson, graesslin, sebas, #kwin, dvratil
Cc: kde-frameworks-devel, davidedmundson, plasma-devel, ragreen, Pitel, 
schernikov, michaelh, ZrenBot, ngraham, bruns, alexeymin, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D10040: Add serial number and EISA ID to OutputDevice interface

[Daniel Vrátil]

dvratil added a comment.


  In D10040#286064 , @davidedmundson 
wrote:
  
  > @dvratil want me to finish this?
  
  
  @davidedmundson  Yes, I'd appreciate it. I won't have time to look into this 
any time soon, sorry :(

REPOSITORY
  R127 KWayland

REVISION DETAIL
  https://phabricator.kde.org/D10040

To: dvratil, graesslin, sebas, #kwin
Cc: kde-frameworks-devel, davidedmundson, plasma-devel, ragreen, Pitel, 
schernikov, michaelh, ZrenBot, ngraham, bruns, alexeymin, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D10040: Add serial number and EISA ID to OutputDevice interface

[David Edmundson]

davidedmundson added a comment.


  In another patch I'm doing a V2 of this interface in the next frameworks 
cycle.
  May as well try to merge this at the same time so we only have 1 version bump?
  
  @dvratil want me to finish this?

REPOSITORY
  R127 KWayland

REVISION DETAIL
  https://phabricator.kde.org/D10040

To: dvratil, graesslin, sebas, #kwin
Cc: kde-frameworks-devel, davidedmundson, plasma-devel, ragreen, Pitel, 
schernikov, michaelh, ZrenBot, ngraham, bruns, alexeymin, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D10040: Add serial number and EISA ID to OutputDevice interface

[David Edmundson]

davidedmundson added a reviewer: KWin.
Restricted Application edited subscribers, added: kde-frameworks-devel; 
removed: Frameworks.

REPOSITORY
  R127 KWayland

REVISION DETAIL
  https://phabricator.kde.org/D10040

To: dvratil, graesslin, sebas, #kwin
Cc: kde-frameworks-devel, davidedmundson, plasma-devel, ragreen, Pitel, 
schernikov, michaelh, ZrenBot, ngraham, bruns, alexeymin, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein, 
#frameworks

D10040: Add serial number and EISA ID to OutputDevice interface

[Martin Flöser]

graesslin requested changes to this revision.
graesslin added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> outputdevice.xml:104-107
> + + summary="textual representation of serial number"/>
> + + summary="textual representation of EISA identifier"/>

I'm not sure whether it's allowed to add arguments to an existing event. This 
would result in incompatibilities. You can make the server only emit to clients 
having the version, but then you actually broke any client which only has 
version 1.

To be really compatible you need to add a new event.

REPOSITORY
  R127 KWayland

REVISION DETAIL
  https://phabricator.kde.org/D10040

To: dvratil, graesslin, sebas
Cc: davidedmundson, plasma-devel, #frameworks, schernikov, ZrenBot, ngraham, 
alexeymin, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, 
sebas, apol, mart, hein

D10040: Add serial number and EISA ID to OutputDevice interface

[Daniel Vrátil]

dvratil marked 2 inline comments as done.
dvratil added inline comments.

INLINE COMMENTS

> davidedmundson wrote in outputdevice_interface.cpp:450
> You're not actually sending the serial number or eisa anywhere..
> 
> The others do it in sendGeometry (including the manufacturer)
> 
> When you do add it, you need to check the client is bound with version 2, 
> otherwise you'll crash the client.
> 
> Something like
>  if(wl_proxy_get_version(resourceOfBoundClient) >= 
> ORG_KDE_KWIN_SETSERIAL_SINCE_VERSION) {
> 
>   org_kde_output_...

I am sending the stuff in `sendGeometry()` (see right above)

REPOSITORY
  R127 KWayland

REVISION DETAIL
  https://phabricator.kde.org/D10040

To: dvratil, graesslin, sebas
Cc: davidedmundson, plasma-devel, #frameworks, schernikov, ZrenBot, ngraham, 
alexeymin, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, 
sebas, apol, mart, hein

D10040: Add serial number and EISA ID to OutputDevice interface

[Daniel Vrátil]

dvratil updated this revision to Diff 25806.
dvratil marked an inline comment as done.
dvratil added a comment.


  Fix versioning

REPOSITORY
  R127 KWayland

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10040?vs=25802&id=25806

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D10040

AFFECTED FILES
  src/client/outputdevice.cpp
  src/client/outputdevice.h
  src/client/protocols/outputdevice.xml
  src/server/outputdevice_interface.cpp
  src/server/outputdevice_interface.h

To: dvratil, graesslin, sebas
Cc: davidedmundson, plasma-devel, #frameworks, schernikov, ZrenBot, ngraham, 
alexeymin, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, 
sebas, apol, mart, hein

D10040: Add serial number and EISA ID to OutputDevice interface

[Daniel Vrátil]

dvratil added a dependent revision: D10042: Parse EDID on the backend side.

REPOSITORY
  R127 KWayland

REVISION DETAIL
  https://phabricator.kde.org/D10040

To: dvratil, graesslin, sebas
Cc: davidedmundson, plasma-devel, #frameworks, schernikov, ZrenBot, ngraham, 
alexeymin, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, 
sebas, apol, mart, hein

D10040: Add serial number and EISA ID to OutputDevice interface

[David Edmundson]

davidedmundson added inline comments.

INLINE COMMENTS

> outputdevice.xml:32
>  
>  
>  

you need to bump this

> outputdevice_interface.cpp:450
>  SETTER(setModel, const QString&, model)
> +SETTER(setSerialNumber, const QString&, serialNumber)
> +SETTER(setEisaId, const QString&, eisaId)

You're not actually sending the serial number or eisa anywhere..

The others do it in sendGeometry (including the manufacturer)

When you do add it, you need to check the client is bound with version 2, 
otherwise you'll crash the client.

Something like
 if(wl_proxy_get_version(resourceOfBoundClient) >= 
ORG_KDE_KWIN_SETSERIAL_SINCE_VERSION) {

  org_kde_output_...

REPOSITORY
  R127 KWayland

REVISION DETAIL
  https://phabricator.kde.org/D10040

To: dvratil, graesslin, sebas
Cc: davidedmundson, plasma-devel, #frameworks, schernikov, ZrenBot, ngraham, 
alexeymin, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, 
sebas, apol, mart, hein

D10040: Add serial number and EISA ID to OutputDevice interface

[Daniel Vrátil]

dvratil added a dependent revision: D10041: Improve EDID information on Wayland.

REPOSITORY
  R127 KWayland

REVISION DETAIL
  https://phabricator.kde.org/D10040

To: dvratil, graesslin, sebas
Cc: plasma-devel, #frameworks, schernikov, ZrenBot, ngraham, alexeymin, 
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, 
apol, mart, hein

D10040: Add serial number and EISA ID to OutputDevice interface

[Daniel Vrátil]

dvratil created this revision.
dvratil added reviewers: graesslin, sebas.
Restricted Application added subscribers: Frameworks, plasma-devel.
Restricted Application added projects: Plasma on Wayland, Frameworks.
dvratil requested review of this revision.

REPOSITORY
  R127 KWayland

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D10040

AFFECTED FILES
  src/client/outputdevice.cpp
  src/client/outputdevice.h
  src/client/protocols/outputdevice.xml
  src/server/outputdevice_interface.cpp
  src/server/outputdevice_interface.h

To: dvratil, graesslin, sebas
Cc: plasma-devel, #frameworks, schernikov, ZrenBot, ngraham, alexeymin, 
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, 
apol, mart, hein