Define default Action when we plug unknown monitor
AbandonedPublic

Authored by mlaurent on Oct 24 2017, 12:23 PM.

Details

Reviewers
davidedmundson
Summary

Default action when we plug unknown monitor

Test Plan

Diff Detail

Repository
R104 KScreen
Branch
customize_connect_unknown_monitor
Lint
No Linters Available
Unit
No Unit Test Coverage
mlaurent created this revision.Oct 24 2017, 12:23 PM

Looks like this was opened by mistake; you've already got D8442. Can this be closed?

Wierd why it created a new review ???
This one is more recent.
I will close 8442

Unless I'm misreading things, I think there's one part that's going to appear very confusing.

I have my (small) laptop. Lets say I want any new (big) screen to appear on the left. So I save "Extend to left".
So I'd expect to see: monitor, laptop.

What will happen, is it will extend my biggest screen to the left, leaving me with:
laptop, monitor

rendering the option seemingly broken.

mlaurent updated this revision to Diff 21283.Oct 25 2017, 7:53 AM
  • Update extend to left
graesslin added inline comments.
kded/generator.cpp
585

Please don't use qforeach in new code as Qt might deprecate it.

mlaurent updated this revision to Diff 21288.Oct 25 2017, 9:09 AM
  • Don't use Q_FOREACH as it will be deprecated in the future
mlaurent marked an inline comment as done.Oct 25 2017, 9:09 AM
mlaurent retitled this revision from WIP: Define default Action when we plug unknown monitor to Define default Action when we plug unknown monitor.Oct 31 2017, 8:23 AM
davidedmundson requested changes to this revision.Oct 31 2017, 8:32 AM

I don't see that my comment about extending has been addressed at all.

This revision now requires changes to proceed.Oct 31 2017, 8:32 AM
mlaurent edited the test plan for this revision. (Show Details)Oct 31 2017, 8:53 AM
mlaurent edited the test plan for this revision. (Show Details)
mlaurent edited the test plan for this revision. (Show Details)
mlaurent edited the test plan for this revision. (Show Details)
sebas added a subscriber: sebas.Oct 31 2017, 9:00 AM

Once you're done with the code, please also add screenshot so usability can have a look at it. I'm not sure we should add UI for this, the screen setup is already very complex and hard to understand. Perhaps a set of clear use-cases would make this clearer?

No unit tests for the code (I know, hard to do) will add to the maintenance burden, which is already high.

Just seeing the code is definitely not enough for me to ACK this.

sebas added a comment.Oct 31 2017, 9:02 AM

Ow, I might add that we wanted to address a similar thing by providing a popup when a display is connected that asks the user what do do now, and then an action can be chosen. Maybe your time is spent better working on that feature, as we already agreed that this would make sense.

sebas added a comment.Nov 20 2017, 2:12 PM

@mlaurent What's your plan with this?

(I was in vacation.)
Ok so I need to create an osd plasma for it.
I can trash this current patch.

@sebas: What do you want as widget design ?

Is it ok to add plasma dependancy in kscreen ?

I left a comment a month ago. It has not been addressed.

there's one part that's going to appear very confusing.

I have my (small) laptop. Lets say I want any new (big) screen to appear on the left. So I save "Extend to left".
So I'd expect to see: monitor, laptop.

What will happen, is it will extend my biggest screen to the left, leaving me with:
laptop, monitor

rendering the option seemingly broken.

It's artbitrary depending on what size monitor you happen to plug in.

mwolff added a subscriber: mwolff.Dec 6 2017, 12:34 PM

I left a comment a month ago. It has not been addressed.

there's one part that's going to appear very confusing.

I have my (small) laptop. Lets say I want any new (big) screen to appear on the left. So I save "Extend to left".
So I'd expect to see: monitor, laptop.

What will happen, is it will extend my biggest screen to the left, leaving me with:
laptop, monitor

rendering the option seemingly broken.

It's artbitrary depending on what size monitor you happen to plug in.

I agree that this is broken, functionality-wise. But it was broken before too, right? So why was the old code looking up the biggest monitor and extending it, instead of using the primary one?

kded/generator.cpp
328

this cleanup should happen in a separate patch

It was semi broken. Behavior was arbitrary on size, but there's no ui pretending it's anything else.

I'd be happy with this patch if it extended from an in-built monitor (you can't use primary here as we're the ones setting that) instead of biggest.

@sebas suggested we approach it as an OSD that would appear on screen and allow the user to select an action they want to take. I think it's a good approach, even more flexible than an option in the KCM. And IIRC it was also what we planned long time ago when I was working on KScreen, but never got around to implement it :-).

Now the question is how should we approach it code-wise: should we write our own OSD in the KScreen KDED module, or should we extend the Plasmashell OSD code and expose it on the org.kde.osdService interface and only have KScreen interact with it via DBus? What would you guys prefer?

@dvratil Good question. I actually have a branch that adds an OSD to kscreen. It's in sebas/osd right now and I haven't merged it since it behaves weirdly on Wayland (placement is not reliable). That should be the start. The plasmashell OSD is doesn't allow placing it on a specific screen (or both) which is something we'll want for this, since we're messing with screens right during that stage, and it's also not interactive. So we'd have to add quite some cases to plasmashell that are really only useful for kscreen. I'd start with that OSD and add interactivity and the selector for layouts.

@mlaurent The idea is to popup a selector for the different layouts when a new screen is connected and then switch to the chosen layout. It will be remembered so on next connect, this layout will be picked automatically. The OSD should resemble the other Plasma OSDs, for a detailed design, please refer to @jensreuterberg or or other VDG members. There are already icons added to Breeze for that, and enums are in libkscreen as well (IIRC). This adds a runtime dependency on kscreen (not libkscreen), which is entirely fine. Hit me up on IRC or email for additional details.

mlaurent abandoned this revision.May 2 2018, 6:01 AM