[Differential] [Commented On] D2218: New logic for screen numbers in plasmashell
lbeltrame added a comment. In https://phabricator.kde.org/D2218#41975, @mart wrote: > most comments should be adressed now, will need again a period of test with several multiscreen users, as it has changed a bit I've been running this for a while, in two setups (one with 3 identical screens, another with two screens of which one is rotated 90 degrees) with no issues. Coupled with the already-merged patch by sebas in kscreen, I get no containment dance anymore at startup. REPOSITORY rPLASMAWORKSPACE Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D2218 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: mart, #plasma Cc: lbeltrame, davidedmundson, rwooninck, graesslin, plasma-devel, jensreuterberg, abetts, sebas ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
[Differential] [Commented On] D2218: New logic for screen numbers in plasmashell
mart added a comment. most comments should be adressed now, will need again a period of test with several multiscreen users, as it has changed a bit REPOSITORY rPLASMAWORKSPACE Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D2218 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: mart, #plasma Cc: davidedmundson, rwooninck, graesslin, plasma-devel, jensreuterberg, abetts, sebas ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
[Differential] [Commented On] D2218: New logic for screen numbers in plasmashell
mart added a comment. In https://phabricator.kde.org/D2218#41871, @davidedmundson wrote: > Personally, I think the part that's out of place is ScreenPool touching the desktop views; if you move m_desktopViewforId to shellCorona the design all fits: > insert/remove in ScreenPool become reflective > and all DesktopView stuff is within one class, screen to ID mapping is in one class. I tried to move the panels logic in screenpool but i didn't really like it, screenpool kindof became almost a small clone of corona.. I'll try instead moving m_desktopViewforId in corona REPOSITORY rPLASMAWORKSPACE Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D2218 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: mart, #plasma Cc: davidedmundson, rwooninck, graesslin, plasma-devel, jensreuterberg, abetts, sebas ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
[Differential] [Commented On] D2218: New logic for screen numbers in plasmashell
davidedmundson added a comment.
> i may be able to kill idsfordesktopview tough, will update the rr
Yep, that's exactly what I meant. ++
Also means panels can use the API directly. (see comment #3)
> perhaps would be more clear if the logic for that is completely moved in
screenpool?
There should definitely be some consistency. Either screenpool is in charge
of managing views or shellcorona is.
Either woudl be valid but currently this patch current inserts and removals
of DesktopView to the right screen are all handled by ShellCorona setting the
screen, but primary changes of DesktopView are done by ScreenPool; and Panels
are mixed up too.
Personally, I think the part that's out of place is ScreenPool touching the
desktop views; if you move m_desktopViewforId to shellCorona the design all
fits:
insert/remove in ScreenPool become reflective
and all DesktopView stuff is within one class, screen to ID mapping is in one
class.
INLINE COMMENTS
> panelview.cpp:647
> {
> +//it's important the panel is positioned immediately in order to not
> have the
> +//qscreen changed under his feet
well, this is now completely not true as we're following screenToFollow
> shellcorona.cpp:1421
> if (view) {
> QScreen *screen = view->screen();
> +foreach (int id, m_screenPool->ids()) {
surely this should be screenToFollow()?
> shellcorona.cpp:1422
> QScreen *screen = view->screen();
> -for (int i = 0; i < m_views.count(); i++) {
> -if (m_views[i]->screen() == screen) {
> -return i;
> +foreach (int id, m_screenPool->ids()) {
> +if (m_screenPool->view(id)->screen() == screen) {
this is silly.
We're looping through the desktops to find one with the same screen as us -
then looking up the name of that.
We have the screen, we can use m_screenPool->id(screen-name());
REPOSITORY
rPLASMAWORKSPACE Plasma Workspace
REVISION DETAIL
https://phabricator.kde.org/D2218
EMAIL PREFERENCES
https://phabricator.kde.org/settings/panel/emailpreferences/
To: mart, #plasma
Cc: davidedmundson, rwooninck, graesslin, plasma-devel, jensreuterberg, abetts,
sebas
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
[Differential] [Commented On] D2218: New logic for screen numbers in plasmashell
davidedmundson added a comment. Something has got messed up with the patch? REPOSITORY rPLASMAWORKSPACE Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D2218 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: mart, #plasma Cc: davidedmundson, rwooninck, graesslin, plasma-devel, jensreuterberg, abetts, sebas ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
[Differential] [Commented On] D2218: New logic for screen numbers in plasmashell
mart added a comment. In https://phabricator.kde.org/D2218#41550, @davidedmundson wrote: > When adding a new design can you describe the design in words. i.e what roles a class has > It'd make things a lot easier. > > ScreenPool: > It seems to have two jobs: > > - you've got the converting screen "names" to IDs. > - "Tracking" DesktopViews > > Why do we have m_idsForConnector and m_idsForDesktopView > > m_idsForDesktopView is bound to be the same as m_idsForConnector(view->screenToFollow()->name()) right? > > at which point we can kill the second one, and that simplifies a lot of the code and avoid something that can go out of sync. I don't think I can drop idsForConnector, because (as connectorsForId) it contains associations also of screens that it remembers but may be disabled right now (it saves and loads to/from configuration file) i may be able to kill idsfordesktopview tough, will update the rr > Panels are now a bit of a mix: > > In createWaitingPanels we base the panel screen based on where the DesktopView is > in primaryChanged - we don't and use screens API directly (BUG: and setting screenToFollow) views gets switched in screens only in the case the primary screen changes, that's the one moment you want to see panels and desktops moving to a different screen do you think if panels would be completely managed in screenpool as well logic would be more readable? > in removeView we're using the View as the indicator of what screen is which. > screenForContainment is also going via View. > > Is it meant to be going via a View or not? > > Personally I think it's a bit weird as we can generate the index directly from the screen, but whatever it should be it should be consistent what we know there is that a desktopview must be removed due to a screen death, so the panels with that screen should be killed as well. perhaps would be more clear if the logic for that is completely moved in screenpool? > bugs: > I think you also have a potential crash if you switch primary on a panel and then disconnect the first screen hmm, i tried to brutally disconnect the primary screen, but doesn't seem to crash? > This code will have a bug if you drag a panelview to another screen - then resize the first screen. why? when dragging to another screen screentofollow of the panel would be changed as well. > Same for if you have a panel on a primary screen then switch primary then alter the screen. > > You have a crash if: > > - you have previously inserted a containment on screen A > - you reboot with screen B, C > - number of screens ==2, firstAvailableIndex=1 > - the first containment will restore > - the second one will be a new containment with an ID of 3 > - createContainment checks number of ScreensAvailable and will return a null pointer. yes, there should be a new containment as the one of screen a should stay for when/if screen a is connected again (should be fine with latest libplasma) REPOSITORY rPLASMAWORKSPACE Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D2218 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: mart, #plasma Cc: davidedmundson, rwooninck, graesslin, plasma-devel, jensreuterberg, abetts, sebas ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
[Differential] [Commented On] D2218: New logic for screen numbers in plasmashell
davidedmundson added a comment. > You have a crash if: edit, that final crash doesn't exist anymore. Didn't see that you'd removed the numScreens check in p-f REPOSITORY rPLASMAWORKSPACE Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D2218 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: mart, #plasma Cc: davidedmundson, rwooninck, graesslin, plasma-devel, jensreuterberg, abetts, sebas ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
[Differential] [Commented On] D2218: New logic for screen numbers in plasmashell
davidedmundson added a comment. When adding a new design can you describe the design in words. i.e what roles a class has It'd make things a lot easier. ScreenPool: It seems to have two jobs: - you've got the converting screen "names" to IDs. - "Tracking" DesktopViews Why do we have m_idsForConnector and m_idsForDesktopView m_idsForDesktopView is bound to be the same as m_idsForConnector(view->screenToFollow()->name()) right? at which point we can kill the second one, and that simplifies a lot of the code and avoid something that can go out of sync. Panels are now a bit of a mix: In createWaitingPanels we base the panel screen based on where the DesktopView is in primaryChanged - we don't and use screens API directly (BUG: and setting screenToFollow) in removeView we're using the View as the indicator of what screen is which. screenForContainment is also going via View. Is it meant to be going via a View or not? Personally I think it's a bit weird as we can generate the index directly from the screen, but whatever it should be it should be consistent bugs: I think you also have a potential crash if you switch primary on a panel and then disconnect the first screen This code will have a bug if you drag a panelview to another screen - then resize the first screen. Same for if you have a panel on a primary screen then switch primary then alter the screen. You have a crash if: - you have previously inserted a containment on screen A - you reboot with screen B, C - number of screens ==2, firstAvailableIndex=1 - the first containment will restore - the second one will be a new containment with an ID of 3 - createContainment checks number of ScreensAvailable and will return a null pointer. REPOSITORY rPLASMAWORKSPACE Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D2218 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: mart, #plasma Cc: davidedmundson, rwooninck, graesslin, plasma-devel, jensreuterberg, abetts, sebas ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
[Differential] [Commented On] D2218: New logic for screen numbers in plasmashell
rwooninck added a comment. I have been using this patch for a couple of days now and I haven’t experienced issues anymore with the screens. Before the patch, I had the issue that despite the correct kscreen configuration, the panel would start on the wrong screen, etc. (This was then corrected by restarting plasmashell). Also from time to time I had the issue that one of the two screens would be completely black and no background or panel would be displayed. Both issues have no longer occurred, despite using three different setups (Single laptop screen, Home with external monitor as primary and laptop screen on the right, Work with external monitor as primary and laptop screen on the left). Everything works now as it should. REPOSITORY rPLASMAWORKSPACE Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D2218 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: mart, #plasma Cc: rwooninck, graesslin, plasma-devel, jensreuterberg, abetts, sebas ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
[Differential] [Commented On] D2218: New logic for screen numbers in plasmashell
mart added inline comments. INLINE COMMENTS > graesslin wrote in panelview.cpp:679-684 > what about that connect? right, that didn't work, it tried to be too smart and moved the panel around, while now when m_screenToFollow goes away, the panel view gets deleted externally in ShellCorona::removeDesktop REPOSITORY rPLASMAWORKSPACE Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D2218 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: mart, #plasma Cc: graesslin, plasma-devel, jensreuterberg, abetts, sebas ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
