> On Feb. 2, 2016, 12:43 p.m., Sebastian Kügler wrote: > > shell/shellcorona.cpp, line 629 > > <https://git.reviewboard.kde.org/r/126961/diff/1/?file=442514#file442514line629> > > > > m_screenConfiguration may still be a nullptr here, since it's set only > > after GetConfigOperation returns. > > > > That may well be the reason why the sync QScreen's count is used here, > > but this leads to problems down the road, as you note. > > > > We could make sure we have the number of screens returned here by > > making GetConfigOperation sync, so using its exec() and thus blocking in > > the setShell call. > > David Edmundson wrote: > ooh good catch. > > I /think/ a simple if (null) return 0; is semantically correct. > As this should be a count of screens plasma knows about. > > Sebastian Kügler wrote: > That could prevent the crash, but then we'd fire more events than > necessary (most likely, there will be at least one screen, so next thing that > happens is a outputAdded signal). I think op->exec() is the more elegant > solution here, especially since plasmashell has no business if there aren't > any screens connected, we might as well just wait. The wait here can be quite > significant as GetConfigOperation starts a separate process to run the > backend in and then does IPC. (KSCREEN_BACKEND_INPROCESS=1 "fixes" that, but > is dangerous for XCB since we had a lot of crashers there, and looking at > some bugreports, we still have.) > > David Edmundson wrote: > It wouldn't fire any extra events. We don't start adding screens until > config op completes.
Right. Ok, a simple check is fine with me, too. - Sebastian ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126961/#review91946 ----------------------------------------------------------- On Feb. 2, 2016, 2:17 p.m., David Edmundson wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/126961/ > ----------------------------------------------------------- > > (Updated Feb. 2, 2016, 2:17 p.m.) > > > Review request for Plasma. > > > Bugs: 351777 > https://bugs.kde.org/show_bug.cgi?id=351777 > > > Repository: plasma-workspace > > > Description > ------- > > We were mixing KScreen and QScreen API badly. > > Corona.cpp checks we are requesting a containment for a valid screen > if (screen >= 0 && screen < numScreens()) { > > This fails as numScreens() is Qt API based, whereas the signal we're > adding the output for is ShellCorona::addOutput so we have an effective race > condition. > > BUG: 351777 > > > Diffs > ----- > > shell/shellcorona.cpp 762e503bf59fe648fb0f5b76a36229aa43c563e5 > > Diff: https://git.reviewboard.kde.org/r/126961/diff/ > > > Testing > ------- > > Started Plasma on dual screen. > > Ideally we need to do more testing before backporting, as that entire > codebase is a disgrace. > > > Thanks, > > David Edmundson > >
_______________________________________________ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
