feat: avoid duplicated text when assembling user-facing output names
AcceptedPublic

Authored by dvratil on Feb 26 2020, 12:00 PM.

Details

Reviewers
romangg
Group Reviewers
Plasma
Summary

The string format by default is Vendor Model (output), where Vendor
and Model are read from EDID and output is the name of the output
reported by the platform.

This patch adds logic to skip the (output) part if its the same as
Vendor Model.

Diff Detail

Repository
R104 KScreen
Branch
arcpatch-D27675
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 23822
Build 23840: arc lint + arc unit
dvratil created this revision.Feb 26 2020, 12:00 PM
Restricted Application added a project: Plasma. · View Herald TranscriptFeb 26 2020, 12:00 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
dvratil requested review of this revision.Feb 26 2020, 12:00 PM
romangg added a subscriber: romangg.EditedFeb 26 2020, 12:02 PM

Can you give an explicit example?

romangg retitled this revision from feat: Avoid duplicated text when assembling user-facing output names to feat: avoid duplicated text when assembling user-facing output names.Feb 26 2020, 12:03 PM

This is needed for D10042 (which I'm rebasing right now) when even on KWayland the KScreen::Output will have EDID attached (assembled from information provided by KWayland/KWin). Since on KWayland the output name already is Vendor Model adding the information from EDID ends up with the string Vendor Model (Vendor Model), that is in my case Eizo Nanao Corporation DP-5-EV2736W/96211045 (Eizo Nanao Corporation DP-5-EV2736W/96211045), which is really long and kinda pointless.

I am currently working on some projects. But if you don't hear till Sunday from me about this, ping me.

romangg requested changes to this revision.Sun, Mar 8, 8:30 PM

Ok, I see the issue. The question is if Vendor Model is not a bit stupid as a name for an output and if KWin/KWayland should not be a bit more intelligent about choosing a name. Then it could make sense again to have (Vendor model) appended afterwards. For example I have here a monitor for testing which is called on the website: PROLITE XUB2395WSU-B1. But KWin returns as Vendor "IVM" and as name "DP-1-PL2395W/11638JJ60250".

common/utils.cpp
36

Please don't use single character variable names. Also can this be made more explicit? I can't guess directly what the meaning of a, b and s is. I don't think it is that generic, is it?

58–63

Full-stop in the end.

This revision now requires changes to proceed.Sun, Mar 8, 8:30 PM
ahmadsamir added inline comments.
common/utils.cpp
64

If I am reading this correctly, you could keep the old code and use simplified():
const QString outName = output->name().simplified();

then later on:
name = name.simplified();
if (!name.isEmpty() && name != oName) {

return name + QLatin1Char('(') + outName + QLatin1Char(')');

}

dvratil added a comment.EditedTue, Mar 17, 2:17 PM

As you described, the system makes sense on X11 where you get something like HDMI-1 (Sony GigaTV-12345) so you see which device it is and what physical output it's connected to. Since KWin does not use to output names but only the names of the actually connected devices, the needed behavior is different.

I don't know if it's safe/feasible to change this on KWin side or not.

common/utils.cpp
64

Not really, the catch is in the whitespace at the end of name (then "Vendor Model " == "Vendor Model" is never true)

As you described, the system makes sense on X11 where you get something like HDMI-1 (Sony GigaTV-12345) so you see which device it is and what physical output it's connected to. Since KWin does not use to output names but only the names of the actually connected devices, the needed behavior is different.

I don't know if it's safe/feasible to change this on KWin side or not.

Allright, let's go with your solution now and if somebody thinks it can be improved on KWin side it can be revisited later.

Please handle the nitpicks I raised. When you are at it also add two to three comments to the lambda making it clearer what it does in each line and why. Thanks!

dvratil updated this revision to Diff 77837.Tue, Mar 17, 3:24 PM
  • Make the lambda more explicit
  • Address review comments
dvratil updated this revision to Diff 77838.Tue, Mar 17, 3:26 PM
  • Simplify a bit more
romangg accepted this revision.Tue, Mar 31, 2:35 PM

Sorry for late review. I didn't get notified by this bullshit platform (which might be because I disabled all notifications at one point to not get spammed constantly).

common/utils.cpp
46

full-stop

This revision is now accepted and ready to land.Tue, Mar 31, 2:35 PM