Re: Review Request: use _NET_WM_STATE_HIDDEN to check if the window is minimized instead of WM_STATE == ICONIC when possible.
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108308/ --- (Updated Jan. 10, 2013, 6:50 a.m.) Review request for kdelibs, kwin, Plasma, Aaron J. Seigo, and Martin Gräßlin. Changes --- added plasma to CC group - I think they should be in to comment on it. Description --- When setting Keep window thumbnails to Always (Breaks minimization), kwin will keep WM_STATE to be NORMAL when a client is minimized while including _NET_WM_STATE_HIDDEN in its _NET_WM_STATE, as confirmed by ICCCM[1] and Extended Window Manager Hints[2]. However, apart from the expected result (breaks minimization: the client will continue to refresh its content) the minimized window is not shown as minimized in icontasks and pager. These two plasma addons (and probably other addons as well) uses KWindowInfo::isMinimized to determine whether the window is minimized. However, this function threat all window that are not Iconic (WM_STATE != ICONIC) as not minimized, in contradiction to the Extended window manager hints which says, Pagers and similar applications should use _NET_WM_STATE_HIDDEN instead of WM_STATE to decide whether to display a window in miniature representations of the windows on a desktop. This patch correct this behavior and therefore correct the behavior of both pager and icontasks in this situation. [1] http://tronche.com/gui/x/icccm/sec-4.html#s-4.1.3.1 [2] http://standards.freedesktop.org/wm-spec/wm-spec-1.3.html#id2731936 Diffs - kdeui/windowmanagement/kwindowinfo_x11.cpp d983c9a Diff: http://git.reviewboard.kde.org/r/108308/diff/ Testing --- Compiled, pager and icontasks shows minimized windows correctly. Also tested on openbox (+plasma's pager) by Xuetian Weng. Thanks, Yichao Yu
Re: New lockscreen
On Thursday 10 January 2013 19:37:57 Martin Sandsmark wrote: Hi! The new lock screen has some more or less serious regressions, and doesn't seem to be maintained by anyone in particular (one of the regression bugs filed against it is from november, and I don't really see anyone in particular commenting or fixing anything, it only got a handful of commits in december). FYI: I started investigating one bug and are working on it. It's not a trivial one, so after not fixing it instantly I moved it back, but I will work on it. For me KWin has higher priority. If you ask for maintainers: I consider Marco and me as the maintainers of the new screen locker, the old screen saver HACK (which you seem to be using) is something I consider as deprecated as legacy and it would be really awesome if someone who wants to use them would step up to maintain them. I voted for dropping them (see my blog for reasons). Cheers Martin signature.asc Description: This is a digitally signed message part.
Re: Re: New lockscreen
On Friday 11 January 2013 09:28:19 Martin Sandsmark wrote: Well, the old one managed to be both, so IMHO if we remove features it is a regression (though which ones are you talking about?). no, removing features is not a regression. It is the decision to remove the feature. The use case for screen savers does no longer exist or when did you last have a screen which needs to be saved? For background reading I recommend [1]. Cheers Martin [1] http://blog.martin-graesslin.com/blog/2011/04/rethinking-screensavers/ signature.asc Description: This is a digitally signed message part.
Re: Re: Re: New lockscreen
On Friday 11 January 2013 10:12:13 Martin Sandsmark wrote: On Fri, Jan 11, 2013 at 09:49:06AM +0100, Martin Gräßlin wrote: no, removing features is not a regression. It is the decision to remove the feature. The use case for screen savers does no longer exist or when did you last have a screen which needs to be saved? For background reading I recommend [1]. I still have CRT screens, and plasma displays (yes, ironic or something) are highly susceptible to burn-ins as well. LCD displays usually only have transient burn-ins, but it's still annoying. OLED displays seem to be more susceptible to burn-ins than plasma displays according to some sources. So no, the use case hasn't disappeared at all (it's still the same as it was 20 years ago). and what has protecting the screen against burn-ins to do with security? Nothing, right. Btw. we are not the only ones who go the way of removing screen savers in favor of lock screens. The same happened at GNOME and at Microsoft. So somehow the people working on such features came all independently to the same conclusion. Yes sure there are still CRTs around, there are Plasma screens around and somewhen in the future OLEDs might be used which show the problem. Does that mean that we should use a default (because that's what it's all about) which is not optimal for the 99.9 % of our user base that actually uses LCD screens? And I still really like to have the nice asciiquarium to show off whenever I leave my computer. and again that is orthogonal. There is nothing preventing anyone to add an animation to the lock screen. But if you are going to remove screensaver support, at least do that completely, and don't leave a half-broken implementation in place (and mark the bugs as related to it as invalid). the implementation has been kept there AFAIK because people complain that we wanted to remove it. It would be nice if the people who want to have the old screen savers would step up to support the maintenance. Yes it would have been easier to just remove it and would have removed lots of complexitiy. As I wrote it's a HACK and has always been like that. But the discussion again shows that just removing it completely and tell people to use XSS if they want screen savers would have been the right choice. Cheers Martin signature.asc Description: This is a digitally signed message part.
Re: Re: Re: Re: New lockscreen
On Friday 11 January 2013 10:47:40 Martin Sandsmark wrote: On Fri, Jan 11, 2013 at 10:28:22AM +0100, Martin Gräßlin wrote: and what has protecting the screen against burn-ins to do with security? Nothing, right. Which is why the lock screen has usually been activated separately from the screensaver. no, it wasn't. The lock screen had been implemented inside the screen savers. Yes blank screen was just another kind of screensavers. Btw. we are not the only ones who go the way of removing screen savers in favor of lock screens. The same happened at GNOME and at Microsoft. So somehow the people working on such features came all independently to the same conclusion. Windows 8 still has screensavers AFAIK? yes in the same way as we have screensavers. As a legacy option And Gnome is not something to be emulated in the least bit, IMHO. which is not what I wrote. Yes sure there are still CRTs around, there are Plasma screens around and somewhen in the future OLEDs might be used which show the problem. Does that mean that we should use a default (because that's what it's all about) which is not optimal for the 99.9 % of our user base that actually uses LCD screens? Where on earth did you pull that statistic from? And while your arguments may be in favour of using a blank screensaver by default, I think something that is optimal for 100% of our users is better than 99.9%, and I think you agree. and again that is orthogonal. There is nothing preventing anyone to add an animation to the lock screen. Sure, let's just make sure it works properly. As I wrote it's a HACK and has always been like that. Well, they have always been known as screen hacks, yes, but mostly because they are clever graphical hacks AFAIK. :-P -- Martin Sandsmark signature.asc Description: This is a digitally signed message part.
Re: Re: Re: Re: Re: Re: New lockscreen
On Friday 11 January 2013 13:49:00 Martin Sandsmark wrote: On Fri, Jan 11, 2013 at 01:23:04PM +0100, Martin Gräßlin wrote: On Fri, Jan 11, 2013 at 11:37:43AM +0100, Martin Gräßlin wrote: Which is why the lock screen has usually been activated separately from the screensaver. And no, the lock screen was not running in the screensaver process. Martin, please. I did most of the porting to the new architecture and I re- read the code before replying to your mail writing that. Yes, technically the lock is held by a different process which doesn't invalidate what I wrote. So both technically and from a user experience stand point the locking and the screensaver were separate? IMHO that invalidates what you said. We are not talking about the same things. We have three parts here: * locking of screen (no keyboard, no mouse) * not exposing screen content * unlock dialog to me the lock screen is locking the screen plus not exposing the screen content, while unlock is completely unrelated. The old implementation had the not exposing screen content as a screen saver. I have a huge problem if people twist my words. That is not what I have written and not what I have meant. I quote my words: Btw. we are not the only ones who go the way of removing screen savers in favor of lock screens. The same happened at GNOME and at Microsoft. So somehow the people working on such features came all independently to the same conclusion. there is nothing in it that would say that GNOME at any influenced any of our decisions. You're trying to justify our decision (after the fact) by pointing at Gnome's decision? Or am I misunderstanding you? no I'm not justifying anything here. I just wanted to point out that others came to the same conclusion. Nothing more, nothing less. Nothing to interpret into. It's a side-remark as you can see that I start it with Btw. I'm not saying Gnome influenced our decision in the first place, I'm trying to say that we shouldn't need to use Gnome to justify our own decisions. then please write that and don't claim things people haven't said. That the Gnome people decide something does not validate anything, IMHO. well if they got it right, it's right, isn't it? Just that GNOME did something doesn't mean it's wrong. Given your remark it sounds like that which would be very sad. signature.asc Description: This is a digitally signed message part.
Re: Color Management in KDE
On Monday 21 January 2013 21:22:42 John Layt wrote: Thoughts? thanks John for the long and well elaborated mail. It helped me understanding the porblem better. From what you describe the supposed plugin based approach looks sane to me. Having a D-Bus interface and a runtime dependency is the better solution IMHO. Although KWin has color correction in 4.10 I have never been able to even test it due to the dependency chain and my distribution not providing Oyranos but colord is already installed (note KWin has only a runtime dependency to a module I'm unable to build). That's quite a pity and if such an abstraction helps to get color correction to more users it's the way to go. -- Martin Gräßlin signature.asc Description: This is a digitally signed message part.
Review Request 108808: Do not reset opacity if call for GetWindowProperty fails
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108808/ --- Review request for kdelibs and kwin. Description --- Do not reset opacity if call for GetWindowProperty fails The opacity got unconditionally reset to the default value of fully opaque. In case the property has a value of 0 and the call to get the window property fails, the window would be shown again. This issue was spotted with colibri which faded out it's window and got a flash to fully opaque again. I'm sorry that it's difficult to read this patch properly due to the existing mixing of tabs and whitespaces. The patch changes two things: * only updates the value if all checks succeed * initialise the data_ret variable to not cause a crash on free BUG: 314427 FIXED-IN: 4.10.1 Diffs - kdeui/windowmanagement/netwm.cpp cf28339f90dff1d17ed17842c7c11d8a9718a459 Diff: http://git.reviewboard.kde.org/r/108808/diff/ Testing --- Thanks, Martin Gräßlin
Re: Re: Login for bug reporting
On Thursday 07 February 2013 02:00:19 Teemu Rytilahti wrote: Hi, Martin Graesslin wrote: +1 from me. I don't want reports from users not willing to create an account So easier account creation is also a big no-no? For crashes or for all bugs? That depends on how the easier works. Email address only is for me a big no- no as it means that the user cannot add attachments, which is quite important in the case of a crash trace. Also it *must* be clear to the user that they are interacting with a web software and not sending an email to a person. This is something I see again and again, that users are not aware of that. Mozilla allows one to supply an e-mail address if the user is willing for that, but still allows sending the traces for aggregation. That doesn't apply for regular bugs though... if the backtraces go to a special system where I don't see the dups, I'm all fine. If they go to bugzilla I want less and I mean it. The number of duplicates is a real problem and costs us lot's of time and work. I don't want to see anything done to make it easier. And no, we cannot expect users to recognize a duplicate crash, That's too difficult, we can also not expect users from recognizing whether a backtrace is useful. -- Martin Gräßlin signature.asc Description: This is a digitally signed message part.
Re: Re: Login for bug reporting
On Thursday 07 February 2013 11:14:01 Frank Reininghaus wrote: If you don't believe me, look at the crashes reported in January [2]. All those which have DUPL, INVA, WAIT, DOWN, UNMA, DOWN, UPST, WORK, BACK in the 'Resolution' column were not useful, but still required at the very, very least a minute or two each to handle them. I once calculated it for our most often reported bug. It's a driver crash, nothing we could do about it. It has 133 duplicates. Workflow: * incoming mail in bugs mail folder * read backtrace * click the link * switch to browser * scroll down to duplicate field * enter kwin-intel * click submit (* curse because Thomas was faster) With the it interrupted the work it takes about 1 to 2 minutes. That's an easy one as we don't have to look for the duplicate. So it's 266 minutes just for this one bug which is half a person day of work. Also spend a moment and look at the report. There is multiple times written that we don't want any further comments on the bug and that doesn't help anything. Still attachements, still duplicates. -- Martin Gräßlin signature.asc Description: This is a digitally signed message part.
Re: Re: cxx11-cmake-modules in kdereview
On Friday 01 March 2013 08:12:59 Thiago Macieira wrote: On sexta-feira, 1 de março de 2013 16.06.38, Ivan Čukić wrote: p.s. Please don't start a discussion on whether C++11 should be used or not, we have had that already, and everything we are doing is in accordance with the decisions made then. Can we discuss whether such module should be used? Why can't you use the #defines from QtCore? because we don't use Qt 5 yet? -- Martin Gräßlin signature.asc Description: This is a digitally signed message part.
Review Request 109282: Drop usage of KWindowSystem::doNotMange from KJavaApplet
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109282/ --- Review request for kdelibs and David Faure. Description --- KWindowSystem::doNotMange is a method which performs a D-Bus call to KWin to inform KWin about a window with a specific title to not manage it. Manage here actually meens to set the window to state hidden, but it's nevertheless properly managed and e.g. appears in the Alt+Tab window switcher and is also shown in the taskbar. It's rather obvious that KHTML should not expect the window manager KWin and should not interact with it through D-Bus - after all it's an X11 window manager and we normally talk X. This hack had been added for the Java applet years ago. The reason for this is that the Java windowing system abstraction AWT is too limited. It doesn't allow direct interaction with the window system causing the Applet window to be first mapped to the screen and then getting embedded. This causes a short flicker which is worked around by the doNotManage hack. To suppress this flicker KHTML now sets the appropriate flag and also the flicker is hardly visible anyway because nowadays our computers are much faster ;-) and we have compositing. The doNotManage call clearly falls too short and therefore this change improves the situation by setting the hidden flag and in addition the skip pager and skip taskbar. This at least makes sure that the applets are not shown in the taskbar, though they are still shown in Alt+Tab. Seems like we never exported the SkipSwitcher flag. Diffs - khtml/java/kjavaappletwidget.cpp e9adc4c Diff: http://git.reviewboard.kde.org/r/109282/diff/ Testing --- installed Java, got it somehow magically configured to even have a plugin, found a website, applet is shown. Thanks, Martin Gräßlin
Re: Review Request 109282: Drop usage of KWindowSystem::doNotMange from KJavaApplet
On March 4, 2013, 9:29 p.m., Thomas Lübking wrote: Reg. switcher skipping: SkipSwitcher isn't NETWM and wasn't added to KWindowSystem / NET because of the libs freeze. Also see bug #191310 One could pass the window a type like NET::Override (while that's honest but not NETWM compliant) or eg. NET::Desktop or NET::Dock (though bumping it to the above layer until it's re-embedded) However, *actually* when a (managed) window gets embedded, kwin should unmanage it (thus drop it from the focuschain, thus switcher) since we don't care about non-toplevel windows. Thus i actually don't see why the window would need to be set NET::Hidden | NET::SkipTaskbar | NET::SkipPager at all. It's about to be embedded the very next moment and the flicker has occurred at this point as well. Reg. flicker: what *could* (- means: i don't like that too much) be done is to schedule every maprequest for some 50ms (by this also managing) what would also protect us reg. bug #192470 However, *actually* when a (managed) window gets embedded, kwin should unmanage it (thus drop it from the focuschain, thus switcher) since we don't care about non-toplevel windows. that's something that surprised me as it just doesn't happen. Quite likely a bug in KWin Reg. flicker: what *could* (- means: i don't like that too much) be done is to schedule every maprequest for some 50ms the fade effect does kind of take care of it. To really see the flicker I had to add a 5 sec sleep before embedding - Martin --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109282/#review28554 --- On March 4, 2013, 8:29 p.m., Martin Gräßlin wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109282/ --- (Updated March 4, 2013, 8:29 p.m.) Review request for kdelibs and David Faure. Description --- KWindowSystem::doNotMange is a method which performs a D-Bus call to KWin to inform KWin about a window with a specific title to not manage it. Manage here actually meens to set the window to state hidden, but it's nevertheless properly managed and e.g. appears in the Alt+Tab window switcher and is also shown in the taskbar. It's rather obvious that KHTML should not expect the window manager KWin and should not interact with it through D-Bus - after all it's an X11 window manager and we normally talk X. This hack had been added for the Java applet years ago. The reason for this is that the Java windowing system abstraction AWT is too limited. It doesn't allow direct interaction with the window system causing the Applet window to be first mapped to the screen and then getting embedded. This causes a short flicker which is worked around by the doNotManage hack. To suppress this flicker KHTML now sets the appropriate flag and also the flicker is hardly visible anyway because nowadays our computers are much faster ;-) and we have compositing. The doNotManage call clearly falls too short and therefore this change improves the situation by setting the hidden flag and in addition the skip pager and skip taskbar. This at least makes sure that the applets are not shown in the taskbar, though they are still shown in Alt+Tab. Seems like we never exported the SkipSwitcher flag. Diffs - khtml/java/kjavaappletwidget.cpp e9adc4c Diff: http://git.reviewboard.kde.org/r/109282/diff/ Testing --- installed Java, got it somehow magically configured to even have a plugin, found a website, applet is shown. Thanks, Martin Gräßlin
Re: Review Request 109282: Drop usage of KWindowSystem::doNotMange from KJavaApplet
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109282/ --- (Updated March 5, 2013, 7:26 p.m.) Review request for kdelibs and David Faure. Changes --- further testing showed that with this patch everything behaves like it should: KWin is no longer managing the Java Applet windows - neither in Alt+Tab nor in the list of Clients they are shown. When embedding ends they are managed again by KWin. Description (updated) --- KWindowSystem::doNotMange is a method which performs a D-Bus call to KWin to inform KWin about a window with a specific title to not manage it. Manage here actually meens to set the window to state hidden, but it's nevertheless properly managed and e.g. appears in the Alt+Tab window switcher and is also shown in the taskbar. It's rather obvious that KHTML should not expect the window manager KWin and should not interact with it through D-Bus - after all it's an X11 window manager and we normally talk X. This hack had been added for the Java applet years ago. The reason for this is that the Java windowing system abstraction AWT is too limited. It doesn't allow direct interaction with the window system causing the Applet window to be first mapped to the screen and then getting embedded. This causes a short flicker which is worked around by the doNotManage hack. To suppress this flicker KHTML now sets the appropriate flag and also the flicker is hardly visible anyway because nowadays our computers are much faster ;-) and we have compositing. The doNotManage call clearly falls too short and therefore this change improves the situation by setting the hidden flag and in addition the skip pager and skip taskbar. This at least makes sure that the applets are not shown in the taskbar. Diffs - khtml/java/kjavaappletwidget.cpp e9adc4c Diff: http://git.reviewboard.kde.org/r/109282/diff/ Testing --- installed Java, got it somehow magically configured to even have a plugin, found a website, applet is shown. Thanks, Martin Gräßlin
Re: Review Request 109282: Drop usage of KWindowSystem::doNotMange from KJavaApplet
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109282/#review29054 --- any comments? Otherwise I'm just going ahead and push it into master. - Martin Gräßlin On March 5, 2013, 7:26 p.m., Martin Gräßlin wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109282/ --- (Updated March 5, 2013, 7:26 p.m.) Review request for kdelibs and David Faure. Description --- KWindowSystem::doNotMange is a method which performs a D-Bus call to KWin to inform KWin about a window with a specific title to not manage it. Manage here actually meens to set the window to state hidden, but it's nevertheless properly managed and e.g. appears in the Alt+Tab window switcher and is also shown in the taskbar. It's rather obvious that KHTML should not expect the window manager KWin and should not interact with it through D-Bus - after all it's an X11 window manager and we normally talk X. This hack had been added for the Java applet years ago. The reason for this is that the Java windowing system abstraction AWT is too limited. It doesn't allow direct interaction with the window system causing the Applet window to be first mapped to the screen and then getting embedded. This causes a short flicker which is worked around by the doNotManage hack. To suppress this flicker KHTML now sets the appropriate flag and also the flicker is hardly visible anyway because nowadays our computers are much faster ;-) and we have compositing. The doNotManage call clearly falls too short and therefore this change improves the situation by setting the hidden flag and in addition the skip pager and skip taskbar. This at least makes sure that the applets are not shown in the taskbar. Diffs - khtml/java/kjavaappletwidget.cpp e9adc4c Diff: http://git.reviewboard.kde.org/r/109282/diff/ Testing --- installed Java, got it somehow magically configured to even have a plugin, found a website, applet is shown. Thanks, Martin Gräßlin
Re: Review Request 109809: Extend Mesa 9.1 restriction to SandyBridge
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109809/#review30216 --- the SandyBridge situation seems to be more complex. I am using a SandyBridge myself and are *not* experiencing this issue (just tested again). So a general block all SandyBridge would be too broad. Furthermore I hope that this issue will be resolved upstream till we release 4.9.3 - Martin Gräßlin On April 1, 2013, 2:44 p.m., Michael Palimaka wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109809/ --- (Updated April 1, 2013, 2:44 p.m.) Review request for kde-workspace. Description --- There have been a number of reports that the mesa problems affect SandyBridge, so this extends the restriction to that too. This addresses bugs 313613 and 317427. http://bugs.kde.org/show_bug.cgi?id=313613 http://bugs.kde.org/show_bug.cgi?id=317427 Diffs - kwin/lanczosfilter.cpp 91f701a6af7d2efec9b273a3f3eb3ef3addb6b84 Diff: http://git.reviewboard.kde.org/r/109809/diff/ Testing --- Thanks, Michael Palimaka
Re: Review Request 109809: Extend Mesa 9.1 restriction to SandyBridge
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109809/ --- (Updated April 1, 2013, 7:17 p.m.) Review request for kde-workspace and kwin. Changes --- adding kwin Description --- There have been a number of reports that the mesa problems affect SandyBridge, so this extends the restriction to that too. This addresses bugs 313613 and 317427. http://bugs.kde.org/show_bug.cgi?id=313613 http://bugs.kde.org/show_bug.cgi?id=317427 Diffs - kwin/lanczosfilter.cpp 91f701a6af7d2efec9b273a3f3eb3ef3addb6b84 Diff: http://git.reviewboard.kde.org/r/109809/diff/ Testing --- Thanks, Michael Palimaka
Re: Review Request 109809: Extend Mesa 9.1 restriction to SandyBridge
On April 1, 2013, 7:16 p.m., Martin Gräßlin wrote: the SandyBridge situation seems to be more complex. I am using a SandyBridge myself and are *not* experiencing this issue (just tested again). So a general block all SandyBridge would be too broad. Furthermore I hope that this issue will be resolved upstream till we release 4.9.3 And it's fixed upstream with 62501c3af85089b423218a41a2e2433ac849c2d3. Expected to hit next minor release of Mesa 9.1. - Martin --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109809/#review30216 --- On April 1, 2013, 7:17 p.m., Michael Palimaka wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109809/ --- (Updated April 1, 2013, 7:17 p.m.) Review request for kde-workspace and kwin. Description --- There have been a number of reports that the mesa problems affect SandyBridge, so this extends the restriction to that too. This addresses bugs 313613 and 317427. http://bugs.kde.org/show_bug.cgi?id=313613 http://bugs.kde.org/show_bug.cgi?id=317427 Diffs - kwin/lanczosfilter.cpp 91f701a6af7d2efec9b273a3f3eb3ef3addb6b84 Diff: http://git.reviewboard.kde.org/r/109809/diff/ Testing --- Thanks, Michael Palimaka
Re: Re: Plasma Workspaces 4.11: the last feature release in the 4.x series for kde-workspace
On Thursday 02 May 2013 22:22:36 Kevin Kofler wrote: On Wednesday 01 May 2013 at 09:41:15, Andriy Rysin wrote: If the feature freeze for 4.11 is May 22 and there is no more feature releases for branch 4.x, where all those features from GSoC go? Probably the same as my libplasma PackageKit integration from GSoC 2011: in the digital nirvana and/or in distro patches. :-( @Kevin: could you please stop this nonesense about your GSoC project. Yes we all got that you are frustrated two years ago. Enough is enough! I don't want to ever read about it any more! Obviously we will consider the Qt 5 transition in our GSoC applications. For example one of the ideas for KWin explicitly states that it has to be developed with Qt 5. -- Martin Gräßlin signature.asc Description: This is a digitally signed message part.
Re: Review Request 110430: [Taskbar applet] Added actions to be perfomed on middle click
On May 15, 2013, 8:06 a.m., Aaron J. Seigo wrote: I am not in favour of this patch for a couple of reasons. First, there is a port underway to QML which does not need to be delayed further by adding more features. More importantly, however, middle click is a special case of not the first two mouse buttons (should we support N button mice?) and it adds yet more configuration to an already complex and full configuration dialog. It also conflicts with the meaning of middle click to expand / collapse groups (a fairly hidden feature I also was not particularly happy with tbh). Albert Vaca Cintora wrote: Hello Aaron and thank for your reply. Let me defend the inclusion of this patch from the problems you mention: - Difficulty to port to QML: This feature is implemented in a ~10 lines switch (not counting the GUI-related code), so porting it should not be a problem (I could do it, if needed). - Support for N button mice: The desktop should adapt to the current hardware, and nowadays most mice have 3 buttons (not N, but neither only 2). Moreover, lots of apps have adopted the behaviour of closing tabs with a middle click, so adding this feature for applications in the taskbar is consistent with them. - Complexity of the configuration dialog: I agree that we should try to keep all the configuration dialogs simple, but not adding new features because of that reminds me of Gnome3 ;) I think this is not solution for the long-discussed problem of the user-friendliness. Finally, and more important, it has 2 open bugs (+ 4 duplicates) so there are real users requesting it. If it's not going to be added, those bugs should be marked as wontfix. Aaron J. Seigo wrote: porting it should not be a problem (I could do it, if needed). that is definitely a point in your favor. assuming this feature addition is accepted: there's little point to putting it in the c++ version, however, only to put it later in the qml version (which is supposed to be in 4.11). so putting it directly in the QML version would make the most sense. Moreover, lots of apps have adopted the behaviour of closing tabs with a middle click to point out the obvious: windows are not tabs. this also implies that middle click to close is actually a *good* feature. i think it is for power users .. but i have seen too many instances where these kinds of magic click that button and magically something happens features lead to confusion, and confusion leads to distrust of the system as a whole. yes, the default is to do nothing in your patch (+1 for that), but if sitting down to someone else's system results in wildly unpredictable behaviour ... keep in mind this is not a random component, but a default part of every plasma desktop installation, so it has to work better than most. how much faster is middle click versus right-click-close? fast enough to warrant the risk of surprising behaviour for people who aren't expecting it? Complexity of the configuration dialog: I agree that we should try to keep all the configuration dialogs simple currently that page has 11 settings. ad-hoc testing i've done in the past hints that many of these settings are already not understandable to many users. i really don't want to make this worse. several of the plasmoids have room for more options. the taskbar, folderview, clock and system tray are four that really don't. adding feature over feature is leading to an increasing mess that nobody cares to clean up. if this patch introduced a re-think of the configuration presentation so that it is easier to understand and more approachable, i would consider that a fair trade for accepting a feature i'm not particularly in favour of :) but not adding new features because of that reminds me of Gnome3 for future reference: when i see this kind of statement made, i usually add -1 to my overall weighting. i do not agree with many design decisions in other projects, but design can not be done well if it is primarily directed by not being that other group. common sense and reasoning should be applied to each case without the justification of not like them, otherwise we just create the opposite kind of error. it has 2 open bugs (+ 4 duplicates) so there are real users requesting it. for any product with a large enough user base, given enough time and an open feature request tracker, any possible feature will be requested (usually multiple times). this means that any feature, regardless of intrinsic value, can be justified using this argument. (the least useful case of this i've seen is when people submit the feature request, then later a patch and then use the feature request as evidence it is wanted ;) if this patch
Review Request 110633: Don't report crashes if version is disabled in Bugzilla
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110633/ --- Review request for KDE Runtime, Plasma, Ben Cooksley, and Myriam Schweingruber. Description --- Bugzilla provides a feature to disable versions. This means that the developers do not want to have further reports for this version. Any crash report is by that not helpful any more. So let's just disable reporting crashes for such bugs. If this change gets accepted I intend to backport it to 4.10 and to inform kde-packagers about it to ship it as an update to *all* version they support. This would automatically prevent most duplicates report we get e.g. for KWin. Diffs - drkonqi/reportinterface.cpp 4190c40 Diff: http://git.reviewboard.kde.org/r/110633/diff/ Testing --- See https://bugs.kde.org/show_bug.cgi?id=320217 - the bug was created with version 4.10.60. Afterward the version got disabled and DrKonqi doesn't allow me to report crashes for this version anymore. File Attachments With it disabled http://git.reviewboard.kde.org/media/uploaded/files/2013/05/24/drkonqi-disabled.png Thanks, Martin Gräßlin
Re: Review Request 110633: Don't report crashes if version is disabled in Bugzilla
On May 24, 2013, 11:44 p.m., Jekyll Wu wrote: Bugzilla itself (since 4.2.5) already rejects any attempt against disabled versions. So even without this patch, DrKonqi users won't be able to create crash report against disabled versions in the end. From developers POV, you don't need to worry about that. The problem is usability for users. I'm not sure this reused error dialog is more informative than the existing one in https://bugs.kde.org/attachment.cgi?id=78600action=edit. So I'm against this patch in its current simple form. As said in [1][2], I'm working on a patch for the usability improvement and plan to make it into 4.11. I will create a review request today or tomorrow. [1] https://bugs.kde.org/show_bug.cgi?id=315073#c3 [2] https://bugs.kde.org/show_bug.cgi?id=318769#c1 Bugzilla itself (since 4.2.5) already rejects any attempt against disabled versions. Bugzilla does blcok, but DrKonqi still reports them as unknown version. I know it because I see the crash reports coming in - Martin --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110633/#review33110 --- On May 24, 2013, 4:54 p.m., Martin Gräßlin wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110633/ --- (Updated May 24, 2013, 4:54 p.m.) Review request for KDE Runtime, Plasma, Ben Cooksley, and Myriam Schweingruber. Description --- Bugzilla provides a feature to disable versions. This means that the developers do not want to have further reports for this version. Any crash report is by that not helpful any more. So let's just disable reporting crashes for such bugs. If this change gets accepted I intend to backport it to 4.10 and to inform kde-packagers about it to ship it as an update to *all* version they support. This would automatically prevent most duplicates report we get e.g. for KWin. Diffs - drkonqi/reportinterface.cpp 4190c40 Diff: http://git.reviewboard.kde.org/r/110633/diff/ Testing --- See https://bugs.kde.org/show_bug.cgi?id=320217 - the bug was created with version 4.10.60. Afterward the version got disabled and DrKonqi doesn't allow me to report crashes for this version anymore. File Attachments With it disabled http://git.reviewboard.kde.org/media/uploaded/files/2013/05/24/drkonqi-disabled.png Thanks, Martin Gräßlin
Re: Review Request 110633: Don't report crashes if version is disabled in Bugzilla
On May 24, 2013, 11:44 p.m., Jekyll Wu wrote: Bugzilla itself (since 4.2.5) already rejects any attempt against disabled versions. So even without this patch, DrKonqi users won't be able to create crash report against disabled versions in the end. From developers POV, you don't need to worry about that. The problem is usability for users. I'm not sure this reused error dialog is more informative than the existing one in https://bugs.kde.org/attachment.cgi?id=78600action=edit. So I'm against this patch in its current simple form. As said in [1][2], I'm working on a patch for the usability improvement and plan to make it into 4.11. I will create a review request today or tomorrow. [1] https://bugs.kde.org/show_bug.cgi?id=315073#c3 [2] https://bugs.kde.org/show_bug.cgi?id=318769#c1 Martin Gräßlin wrote: Bugzilla itself (since 4.2.5) already rejects any attempt against disabled versions. Bugzilla does blcok, but DrKonqi still reports them as unknown version. I know it because I see the crash reports coming in So I'm against this patch in its current simple form. Are you also against into backporting it to prevent that we stop getting useless crash reports? I worked on this for fixing a real world problem. Just look at: https://bugs.kde.org/buglist.cgi?list_id=661927bug_severity=crashchfieldto=Nowquery_format=advancedchfield=[Bug%20creation]chfieldfrom=2013-01-01version=unspecifiedlongdesc=kwin%20%284.8product=kwinlongdesc_type=allwordssubstr All crashes reported this year against KWin 4.8. Yes the dialog might not be best, but I cannot change it because it's in string freeze. I agree that for 4.11 something better should be done, but this is more for 4.10 and older (especially older). - Martin --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110633/#review33110 --- On May 24, 2013, 4:54 p.m., Martin Gräßlin wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110633/ --- (Updated May 24, 2013, 4:54 p.m.) Review request for KDE Runtime, Plasma, Ben Cooksley, and Myriam Schweingruber. Description --- Bugzilla provides a feature to disable versions. This means that the developers do not want to have further reports for this version. Any crash report is by that not helpful any more. So let's just disable reporting crashes for such bugs. If this change gets accepted I intend to backport it to 4.10 and to inform kde-packagers about it to ship it as an update to *all* version they support. This would automatically prevent most duplicates report we get e.g. for KWin. Diffs - drkonqi/reportinterface.cpp 4190c40 Diff: http://git.reviewboard.kde.org/r/110633/diff/ Testing --- See https://bugs.kde.org/show_bug.cgi?id=320217 - the bug was created with version 4.10.60. Afterward the version got disabled and DrKonqi doesn't allow me to report crashes for this version anymore. File Attachments With it disabled http://git.reviewboard.kde.org/media/uploaded/files/2013/05/24/drkonqi-disabled.png Thanks, Martin Gräßlin
Re: Review Request 110633: Don't report crashes if version is disabled in Bugzilla
On May 24, 2013, 11:44 p.m., Jekyll Wu wrote: Bugzilla itself (since 4.2.5) already rejects any attempt against disabled versions. So even without this patch, DrKonqi users won't be able to create crash report against disabled versions in the end. From developers POV, you don't need to worry about that. The problem is usability for users. I'm not sure this reused error dialog is more informative than the existing one in https://bugs.kde.org/attachment.cgi?id=78600action=edit. So I'm against this patch in its current simple form. As said in [1][2], I'm working on a patch for the usability improvement and plan to make it into 4.11. I will create a review request today or tomorrow. [1] https://bugs.kde.org/show_bug.cgi?id=315073#c3 [2] https://bugs.kde.org/show_bug.cgi?id=318769#c1 Martin Gräßlin wrote: Bugzilla itself (since 4.2.5) already rejects any attempt against disabled versions. Bugzilla does blcok, but DrKonqi still reports them as unknown version. I know it because I see the crash reports coming in Martin Gräßlin wrote: So I'm against this patch in its current simple form. Are you also against into backporting it to prevent that we stop getting useless crash reports? I worked on this for fixing a real world problem. Just look at: https://bugs.kde.org/buglist.cgi?list_id=661927bug_severity=crashchfieldto=Nowquery_format=advancedchfield=[Bug%20creation]chfieldfrom=2013-01-01version=unspecifiedlongdesc=kwin%20%284.8product=kwinlongdesc_type=allwordssubstr All crashes reported this year against KWin 4.8. Yes the dialog might not be best, but I cannot change it because it's in string freeze. I agree that for 4.11 something better should be done, but this is more for 4.10 and older (especially older). Ben Cooksley wrote: Perhaps it might be worth requesting a string freeze exemption here? Perhaps it might be worth requesting a string freeze exemption here? We would need that from each and every distribution. It doesn't help that KDE did the freeze exception and Debian is then not accepting the patch because it violates their policy. - Martin --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110633/#review33110 --- On May 24, 2013, 4:54 p.m., Martin Gräßlin wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110633/ --- (Updated May 24, 2013, 4:54 p.m.) Review request for KDE Runtime, Plasma, Ben Cooksley, and Myriam Schweingruber. Description --- Bugzilla provides a feature to disable versions. This means that the developers do not want to have further reports for this version. Any crash report is by that not helpful any more. So let's just disable reporting crashes for such bugs. If this change gets accepted I intend to backport it to 4.10 and to inform kde-packagers about it to ship it as an update to *all* version they support. This would automatically prevent most duplicates report we get e.g. for KWin. Diffs - drkonqi/reportinterface.cpp 4190c40 Diff: http://git.reviewboard.kde.org/r/110633/diff/ Testing --- See https://bugs.kde.org/show_bug.cgi?id=320217 - the bug was created with version 4.10.60. Afterward the version got disabled and DrKonqi doesn't allow me to report crashes for this version anymore. File Attachments With it disabled http://git.reviewboard.kde.org/media/uploaded/files/2013/05/24/drkonqi-disabled.png Thanks, Martin Gräßlin
Re: Review Request 110633: Don't report crashes if version is disabled in Bugzilla
On May 24, 2013, 11:44 p.m., Jekyll Wu wrote:
Bugzilla itself (since 4.2.5) already rejects any attempt against disabled
versions. So even without this patch, DrKonqi users won't be able to create
crash report against disabled versions in the end. From developers POV, you
don't need to worry about that.
The problem is usability for users. I'm not sure this reused error dialog
is more informative than the existing one in
https://bugs.kde.org/attachment.cgi?id=78600action=edit. So I'm against
this patch in its current simple form.
As said in [1][2], I'm working on a patch for the usability improvement and
plan to make it into 4.11. I will create a review request today or
tomorrow.
[1] https://bugs.kde.org/show_bug.cgi?id=315073#c3
[2] https://bugs.kde.org/show_bug.cgi?id=318769#c1
Martin Gräßlin wrote:
Bugzilla itself (since 4.2.5) already rejects any attempt against
disabled versions.
Bugzilla does blcok, but DrKonqi still reports them as unknown version.
I know it because I see the crash reports coming in
Martin Gräßlin wrote:
So I'm against this patch in its current simple form.
Are you also against into backporting it to prevent that we stop getting
useless crash reports? I worked on this for fixing a real world problem. Just
look at:
https://bugs.kde.org/buglist.cgi?list_id=661927bug_severity=crashchfieldto=Nowquery_format=advancedchfield=[Bug%20creation]chfieldfrom=2013-01-01version=unspecifiedlongdesc=kwin%20%284.8product=kwinlongdesc_type=allwordssubstr
All crashes reported this year against KWin 4.8.
Yes the dialog might not be best, but I cannot change it because it's in
string freeze. I agree that for 4.11 something better should be done, but
this is more for 4.10 and older (especially older).
Ben Cooksley wrote:
Perhaps it might be worth requesting a string freeze exemption here?
Martin Gräßlin wrote:
Perhaps it might be worth requesting a string freeze exemption here?
We would need that from each and every distribution. It doesn't help that
KDE did the freeze exception and Debian is then not accepting the patch
because it violates their policy.
Jekyll Wu wrote:
Those reports are all from KDE SC 4.8.5, where kwin version (using
KDE_VERSION_STRING) was in the strange form of 4.8.5 (4.8.5) . For some
strange reason I'm still not aware of, bugs.kde.org fails to reject reports
with that kind of version (probably due the space in the version) as it
should have. I'm well aware of those reports which shouldn't have been
accepted at the first place (since I'm subscribed to kde-bugs-dist@), but I
haven't got the time to do a good investigation. Anyway, those escaped
crash reports are definitely the race cases.
I understand you are annoyed by those kwin crash reports from 4.8.5, but
backporting this simple patch won't help you much: the DrKonqi from KDE SC
4.8.x simply don't contain the code of fetching version info from
bugs.kde.org through the Product.get webservice API, so backing this simple
patch to 4.8.x is useless. I added those code not long time ago, as the
necessary base of my final goal of preventing outdated crash reports.
So if your goal is not seeing those kwin 4.8.5 crash reports anymore,
there are two quick solutions:
1. apply this simple drkonqi patch to 4.10.x, then ask distribution
packagers(I'm think of Debian Stable) to ship DrKonqi from KDE SC 4.10.4
together with other components from KDE SC 4.8.4/5. I'm not sure how
distribution packagers will think of this kind of mixture.
2. prepare a one-line patch to make kwin from KDE 4.8.5 use version
4.8.5 explicitly, instead of using KDE_VERSION_STRING. That way,
bugs.kde.org itself will reject those crash reports as it has done for
4.9.{0,1,2,3} . Then ask distribution packers to apply this kwin one-line
patch. I think this is more acceptable for distribution packagers. Or maybe
that one line patch should be created against kdelibs, changing
KDE_VERSION_STRING to the plain 4.8.5 ?
Or maybe that one line patch should be created against kdelibs, changing
KDE_VERSION_STRING to the plain 4.8.5 ?
That's the place where it needs to be done, I think. KWin doesn't set a version
number it uses KDE_VERSION_STRING. On the other hand fixing in kdelibs won't
fix the problem as it would require to recompile everything.
This is btw. not just a problem for KWin (that's just what I am familiar with),
it applies to all software from KDE SC. The problem of 4.8.4 and 4.8.5 is
common in the complete workspaces and it's not going to go away as it's the
version used in Debian Stable and Ubuntu LTS.
Would creating versions 4.8.4 (4.8.4) and 4.8.5 (4.8.5) in bugzilla fix the
problem?
- Martin
---
This is an automatically generated e
Re: Review Request 110633: Don't report crashes if version is disabled in Bugzilla
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110633/ --- (Updated May 27, 2013, 7:59 a.m.) Status -- This change has been discarded. Review request for KDE Runtime, Plasma, Ben Cooksley, and Myriam Schweingruber. Description --- Bugzilla provides a feature to disable versions. This means that the developers do not want to have further reports for this version. Any crash report is by that not helpful any more. So let's just disable reporting crashes for such bugs. If this change gets accepted I intend to backport it to 4.10 and to inform kde-packagers about it to ship it as an update to *all* version they support. This would automatically prevent most duplicates report we get e.g. for KWin. Diffs - drkonqi/reportinterface.cpp 4190c40 Diff: http://git.reviewboard.kde.org/r/110633/diff/ Testing --- See https://bugs.kde.org/show_bug.cgi?id=320217 - the bug was created with version 4.10.60. Afterward the version got disabled and DrKonqi doesn't allow me to report crashes for this version anymore. File Attachments With it disabled http://git.reviewboard.kde.org/media/uploaded/files/2013/05/24/drkonqi-disabled.png Thanks, Martin Gräßlin
Re: Review Request 110687: DrKonqi should check for disabled version as the very first step in the reporting assistant.
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110687/#review33280 --- Could you please get some feedback from packagers. I'm not sure whether they like words like unmaintained and upgrade. The fact that we as upstream don't accept bugs doesn't mean it's unmaintained by the distro and it's not said that one could upgrade (think of Debian Stable). - Martin Gräßlin On May 28, 2013, 1:01 p.m., Jekyll Wu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110687/ --- (Updated May 28, 2013, 1:01 p.m.) Review request for KDE Runtime and George Kiagiadakis. Description --- As I have said in https://bugs.kde.org/show_bug.cgi?id=315073#c3 , Bugzilla's new and nice behavior (since 4.2.5) of rejecting reports against disabled versions brings a new usability problem to DrKonqi: users spend value time in downloading debug symbols, generating the backtrace, writing all information he/she can recall, but in the end only to find an error dialog telling them (in a not so clear and friendly way) that bugs.kde.org won't accept his/her report. I would propose making version checking the very first step in the reporting assistant: a perfect bug report against an outdated version is still useless. The patch has been created for sometime and works, but unfortunately I don't have much time for coding since then, so it is not as good as what I have planned to make it to be. Nevertheless, I think it is still a good improvement of the current situation. This addresses bug 315073. http://bugs.kde.org/show_bug.cgi?id=315073 Diffs - drkonqi/CMakeLists.txt 39833d7 drkonqi/drkonqi_globals.h d5f098a drkonqi/productmapping.h f926c9d drkonqi/productmapping.cpp f4e59e5 drkonqi/reportassistantdialog.cpp c296059 drkonqi/reportassistantpages_bugzilla_versioncheck.h PRE-CREATION drkonqi/reportassistantpages_bugzilla_versioncheck.cpp PRE-CREATION drkonqi/reportinterface.h c7e374e drkonqi/reportinterface.cpp 4190c40 drkonqi/ui/assistantpage_versioncheck.ui PRE-CREATION Diff: http://git.reviewboard.kde.org/r/110687/diff/ Testing --- File Attachments rejecting disabled version http://git.reviewboard.kde.org/media/uploaded/files/2013/05/28/drkonqi-version-checking.png Thanks, Jekyll Wu
Re: Review Request 111042: Upstream SUSE specific changes and introduce a DISTRO switch to enable
On June 15, 2013, 7:06 p.m., Albert Astals Cid wrote: Is this some kind of joke? Honestly upstreaming patches is good, but when they make sense, and adding billions of If #SUSE to our code does not make any sense. Please discard this review, break stuff in separate patches and if they make sense we can discuss adding them upstream without the IF SUSE. Johannes Obermayr wrote: Do you remember on http://tsdgeos.blogspot.de/2010/05/complex-systems-are-complex.html and how long I had to study openSUSE's kdelibs4 package which of the patches were applied and which not, rebuilding packages, installing -debuginfo, etc.? Last sentence is one of the reasons for my wish to get also distro specific patches upstream - not only openSUSE's. If I remember right there were and should be some distro specific bug reports on bko: The more people have easy access to source code in Complex systems [which] are complex the likelier reasons for specific bugs can be tracked down ... Yes, binding upstream developers to downstream changes/problems is not really nice but could also mean a win-win-situation for both: downstream devs will do more upstream work and upstream devs can review downstream work, better find distro specific bugs and assign them to downstream devs. Intention of this review request is to start a discussion ... Upstreaming the patches with if #SUSE doesn't solve the problem: one still doesn't know which code is run by a user's setup. Even worse: the chances for bitrot increase. The current system of downstream patches mean that the distribution has to ensure that the patches apply on new versions. A human has to look at each of them. That's the cost of having downstream patches. If you currently not do that (e.g. debug areas for non shipped software) you should rethink whether you have too many patches. For downstream patches there are the following possible solutions: * Try to upstream them: if upstream accepts them you get rid of the patch, if upstream doesn't accept you should best drop the patch, too * For distro-specific behavior: add abstraction with plugin system and upstream * keep patches for build issues (common example: compile errors with non C++ compliant compilers) Personally I would go so far that we as upstream say that we don't accept bug reports in case the distributions carries downstream patches. - Martin --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111042/#review34388 --- On June 15, 2013, 6:27 p.m., Johannes Obermayr wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111042/ --- (Updated June 15, 2013, 6:27 p.m.) Review request for kdelibs. Description --- Distributions should upstream their patches / changes: - Upstream / other distributions can easily see distro specific changes and enable them by default by removing #if defined(DISTRO_xxx) - Maybe duplicate work can be avoided and other distributions can easily use them by || defined(DISTRO_xxx) - Less adaptions of downstream patches ... Diffs - CMakeLists.txt 705c84e kdecore/config/kconfig.cpp 048605d kdecore/config/kconfig_p.h 7751242 kdecore/config/kconfigdata.h e5dd7da kdecore/config/kconfiggroup.h 8eddfd5 kdecore/config/kconfiggroup.cpp 9e73eb7 kdecore/config/kdesktopfile.h 1c4eae6 kdecore/config/kdesktopfile.cpp 54e5910 kdecore/kdebug.areas 29a4415 kdecore/localization/klocale_kde.cpp b010e74 kdecore/services/kservice.h 3843bad kdecore/services/kservice.cpp e2cc86f kdecore/services/kservicegroup.h 9fdf2b0 kdecore/services/kservicegroup.cpp 08bc587 kdecore/services/kservicegroup_p.h 5f21497 kded/vfolder_menu.cpp f0b0b35 kdesu/defaults.h 706a088 kdeui/kernel/kglobalsettings.cpp 2e3a7eb khtml/html/html_objectimpl.cpp f0f590d kio/CMakeLists.txt 4854829 kio/kio/kprotocolmanager.cpp 05bb547 kio/kio/krun.cpp ad5656e kjs/collector.cpp cdd8421 plasma/containment.h e725a99 plasma/containment.cpp fc2ca70 plasma/private/containment_p.h 75a6f80 plasma/theme.cpp 4554de7 suseinstall/CMakeLists.txt PRE-CREATION suseinstall/kbuildsycocaprogressdialog.h PRE-CREATION suseinstall/kbuildsycocaprogressdialog.cpp PRE-CREATION suseinstall/ksuseinstall.h PRE-CREATION suseinstall/ksuseinstall.cpp PRE-CREATION suseinstall/ksuseinstall_export.h PRE-CREATION Diff: http://git.reviewboard.kde.org/r/111042/diff/ Testing --- Thanks, Johannes Obermayr
Re: Review Request 111171: Deprecate (in)active(Title/Text)Color in favor of KColorScheme
On June 22, 2013, 12:58 p.m., Thomas Lübking wrote: kdeui/util/kglobalsettings.cpp, line 308 http://git.reviewboard.kde.org/r/71/diff/1/?file=165092#file165092line308 These are the colors for the window titlbar (with ambiguous function names, though), the default for activeTitleColor used to be #30AEE8 - that is blue! Now you want to return the active window background (ie. usually gray) - iow: enforce the color alignment that once caused the former KWin maintainer to for the oxygen deco into the new default ozone? I mean, decorations can still keep their internal color settings (what was somehow required because the titlebar color was initially happily removed from the color kcm - instead one could define the button color in listviews...), but the implication of this change is that the titlbar can no more be individually configured and *has* to align to the window background. - The RR should clearly state that central color configuration for titlebars will ultimately be unsupported. Until then, the change can imo not go in before KF5 anyway, because it's a very visible behavioral change for users of Laptop, Plastik, BII and some legacy decorations (you still /can/ compile Quartz and Keramik and what they all were called) and pot. some distro specific decorations. Aleix Pol Gonzalez wrote: Are you sure these are being used? because I searched for uses of this API and I didn't see such uses. Thomas Lübking wrote: KWin reads the setting values by hand in libkdecorations/kdecoration_p.cpp - so actually I was wrong in that aspect: the change in KGlobalSettings will not have a visible impact here - no guarantee for 3rd party decos beyond bespin (esp. on custom config dialogs) Not sure whether this makes the change better, since now KWin will no longer reflect what KGlobalSettings reports. I'm not sure how important it is to export those values via KGlobalSettings (i guess they existed to use the colors on MDI windows from KStyle), but even while deprecated, their behavior should not change. And unless the conclusion is that titlebars shall align to the window content color, the advice should also not be to query that instead. It's better to state information not avialable than to give a false information. It's worse - KWin does not just read the values by itself, it reads them from kwinrc instead of kdeglobals. So whatever is set in KGlobalSettings: KWin ignores it. - Martin --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/71/#review34861 --- On June 22, 2013, 12:25 p.m., Àlex Fiestas wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/71/ --- (Updated June 22, 2013, 12:25 p.m.) Review request for KDE Frameworks and kdelibs. Description --- Deprecate: inactiveTitleColor, inactiveTextColor, activeTitleColor, activeTextColor in favor of KColorScheme and replace the implementation of those methods with it. Diffs - kdeui/util/kglobalsettings.h 4b77ed5 kdeui/util/kglobalsettings.cpp 3e60632 khtml/misc/helper.cpp dccb9bf Diff: http://git.reviewboard.kde.org/r/71/diff/ Testing --- I have compared the colors returned by the methods before and after this patch, they are close enough. Additionally used some apps like filelight with the change, and it seems to work for them as well. Thanks, Àlex Fiestas
Re: Review Request 111171: Deprecate (in)active(Title/Text)Color in favor of KColorScheme
On June 22, 2013, 12:58 p.m., Thomas Lübking wrote: kdeui/util/kglobalsettings.cpp, line 308 http://git.reviewboard.kde.org/r/71/diff/1/?file=165092#file165092line308 These are the colors for the window titlbar (with ambiguous function names, though), the default for activeTitleColor used to be #30AEE8 - that is blue! Now you want to return the active window background (ie. usually gray) - iow: enforce the color alignment that once caused the former KWin maintainer to for the oxygen deco into the new default ozone? I mean, decorations can still keep their internal color settings (what was somehow required because the titlebar color was initially happily removed from the color kcm - instead one could define the button color in listviews...), but the implication of this change is that the titlbar can no more be individually configured and *has* to align to the window background. - The RR should clearly state that central color configuration for titlebars will ultimately be unsupported. Until then, the change can imo not go in before KF5 anyway, because it's a very visible behavioral change for users of Laptop, Plastik, BII and some legacy decorations (you still /can/ compile Quartz and Keramik and what they all were called) and pot. some distro specific decorations. Aleix Pol Gonzalez wrote: Are you sure these are being used? because I searched for uses of this API and I didn't see such uses. Thomas Lübking wrote: KWin reads the setting values by hand in libkdecorations/kdecoration_p.cpp - so actually I was wrong in that aspect: the change in KGlobalSettings will not have a visible impact here - no guarantee for 3rd party decos beyond bespin (esp. on custom config dialogs) Not sure whether this makes the change better, since now KWin will no longer reflect what KGlobalSettings reports. I'm not sure how important it is to export those values via KGlobalSettings (i guess they existed to use the colors on MDI windows from KStyle), but even while deprecated, their behavior should not change. And unless the conclusion is that titlebars shall align to the window content color, the advice should also not be to query that instead. It's better to state information not avialable than to give a false information. Martin Gräßlin wrote: It's worse - KWin does not just read the values by itself, it reads them from kwinrc instead of kdeglobals. So whatever is set in KGlobalSettings: KWin ignores it. Thomas Lübking wrote: No ;-) Welcome to the magics of KConfig. I for one have no [WM] entry in kwinrc, it's only in kdeglobal - try grepping it =) However, KConfig resolves to kdeglobal for *every* rc unless explicitly told do do not, so advert kconfig kdeglobal/WM list and kconfig kwinrc/WM list /advert will print the same value (read from kdeglobal) unless you explicitly add a configuration to kwinrc (what's afaics not done anywhere - hopefully ;-) wtf?!? I hate magic functionality - Martin --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/71/#review34861 --- On June 22, 2013, 12:25 p.m., Àlex Fiestas wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/71/ --- (Updated June 22, 2013, 12:25 p.m.) Review request for KDE Frameworks and kdelibs. Description --- Deprecate: inactiveTitleColor, inactiveTextColor, activeTitleColor, activeTextColor in favor of KColorScheme and replace the implementation of those methods with it. Diffs - kdeui/util/kglobalsettings.h 4b77ed5 kdeui/util/kglobalsettings.cpp 3e60632 khtml/misc/helper.cpp dccb9bf Diff: http://git.reviewboard.kde.org/r/71/diff/ Testing --- I have compared the colors returned by the methods before and after this patch, they are close enough. Additionally used some apps like filelight with the change, and it seems to work for them as well. Thanks, Àlex Fiestas
Re: All KConfig files inherit kdeglobals keys by default, good or bad?
On Tuesday 25 June 2013 10:10:03 Aurélien Gâteau wrote: Eventually the default actually should have been CascadeOnly (because IncludeGlobals seems mostly interesting to libs only). I agree, especially because I assume all the code which benefits from inheriting from kdeglobals has been written with reading config from kdeglobals in mind. As such, I think this code should opt-in to inherit from kdeglobals, instead of expecting all code reading configuration to opt-out from it. This cannot be changed in kdelibs4, but would it make sense to change it in KF5? /me agrees I think this behavior was the reason why I first got it wrong that KWin uses the values and called this magic behavior. I was only reading the code and didn't expect that it would include the kdeglobals. Yes it's documented and yes I probably read the documentation in the past, so I should have known, but from reading the code one doesn't see it, so it's surprising. Cheers Martin signature.asc Description: This is a digitally signed message part.
Re: Review Request 111342: make kinfocenter compile on non x11 systems and Windows
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111342/#review35347 --- Not that I could comment on whether it's useful or not on Windows, but my recommendation is to remove the complete Graphical Information subtree on Windows. kinfocenter/Modules/base/os_base.h http://git.reviewboard.kde.org/r/111342/#comment25898 why is this else part needed? If something is still accessing Display it should probably better be ifdefed there. - Martin Gräßlin On July 1, 2013, 1:33 p.m., Patrick von Reth wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111342/ --- (Updated July 1, 2013, 1:33 p.m.) Review request for kde-workspace and kwin. Description --- make kinfocenter compile on non x11 systems and Windows Kinfocenter is quite useful to test a solid backend Diffs - CMakeLists.txt 57cd82c56539b93fafe7866a259c155eebcc86a0 kinfocenter/Modules/CMakeLists.txt 0a87eb48d97df2e0224819225ba0af6bf0d93f39 kinfocenter/Modules/base/os_base.h f09202d9d0c592238735dc1b2d5041a921358adb kinfocenter/Modules/devinfo/soldevicetypes.cpp d3387d972b14368e9fa2b5ad1f97d5210d2beb01 Diff: http://git.reviewboard.kde.org/r/111342/diff/ Testing --- windows Thanks, Patrick von Reth
Re: Review Request 111342: make kinfocenter compile on non x11 systems and Windows
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111342/#review35374 --- from my side it looks OK, though I won't give a ship-it. This is a decision to the kinfocenter developers whether they are fine with the ifdefs. - Martin Gräßlin On July 1, 2013, 3:14 p.m., Patrick von Reth wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111342/ --- (Updated July 1, 2013, 3:14 p.m.) Review request for kde-workspace and kwin. Description --- make kinfocenter compile on non x11 systems and Windows Kinfocenter is quite useful to test a solid backend Diffs - CMakeLists.txt 57cd82c56539b93fafe7866a259c155eebcc86a0 kinfocenter/Modules/CMakeLists.txt 0a87eb48d97df2e0224819225ba0af6bf0d93f39 kinfocenter/Modules/base/os_base.h f09202d9d0c592238735dc1b2d5041a921358adb kinfocenter/Modules/devinfo/soldevicetypes.cpp d3387d972b14368e9fa2b5ad1f97d5210d2beb01 Diff: http://git.reviewboard.kde.org/r/111342/diff/ Testing --- windows Thanks, Patrick von Reth
Re: Disable by default KRandR in 4.11
On Thursday 04 July 2013 01:06:27 Àlex Fiestas wrote: Hi there I want to propose to disable by default KRandR from kde-workspace release for 4.11. +1 Ideally I would like to remove the source code completely, but I guess we are too late into 4.11 to do such thing. why not? You could replace it with a readme to inform people still building from source or packagers not having kscreen yet, that it's time to switch. Cheers Martin signature.asc Description: This is a digitally signed message part.
Re: Re: openSUSE packagers' take on the 3 month release cycle
On Tuesday 09 July 2013 06:33:21 Scott Kitterman wrote: On Tuesday, July 09, 2013 12:05:30 PM Àlex Fiestas wrote: On Monday 08 July 2013 22:01:29 Andrea Scarpino wrote: We don't just run a sed rule on each spec (pkgbuild, in my case) file. We check for new dependencies (resp. dependencies not needed anymore), new modules (resp. modules not part of the SC anymore), build failure, etc... Can't we do something so you don't have to hunt this down but instead just read a list? For build time dependencies, we could do something by looking for find_package, and for runtime dependencies we should figure something out. Our projects are a mess when it comes to runtime dependencies, why don't we fix that for example? How would a run time only dependency be expressed? I've seen some people put them in find_package, which is wrong and then we end up having to patch it away. For build-time dependencies, particularly determining minimum version requirements, I end up reading CMakeLists.txt in my favorite editor. That's not ideal either. what about having this information available in a standardized format, e.g. a dependencies.xml? Cheers Martin signature.asc Description: This is a digitally signed message part.
Re: Re: Releases in 3 months
On Tuesday 09 July 2013 16:12:35 Andras Mantia wrote: I also find the motivation somewhat contradictory. Yes, you want to provide new features faster, but by cutting down testing time. *Are you sure?* Well here we have to ask whether the current testing procedure works. Since the beta got released I have not been running master of the application I maintain. I'm developing ahead in custom branches and don't see a reason why I should switch back to master. I expect that many other developers work in a similar way. Not working ahead in a different branch, means sitting around and doing nothing or wait till a bug report gets in which could be fixed. If you develop in a always releasable master way, the enforced testing period doesn't make any sense. As you say different projects have different development models. If you use master for your development then you need the stabilization time. If you develop in branches, maybe even with an integration branch step before going to master the testing period doesn't give you anything in addition to the larger audience. The testing period slows down the development. Since we are on git, I have started the development of the next release on the day the feature freeze took place. Cheers Martin signature.asc Description: This is a digitally signed message part.
Re: Re: KDE/4.11 branched what to do with kde-workspace?
On Wednesday 10 July 2013 22:39:29 Thomas Lübking wrote: On Mittwoch, 10. Juli 2013 21:58:34 CEST, Albert Astals Cid wrote: Those who want to develop on or use it will hopefully find it and others can continue to use master as it is. (And where important bugfixes etc. for 4.12 can go) But there's not going to be a kde-workspace 4.12 as announced a while ago. No idea what's announced but that's silly and will just cause confusion downstream - we also have kdelibs 4.10 atm and 4.11 soon while technically that's still 4.7 or whatever. There'll have to be (minor) patches to kde-workspace (you cannot keep shipping known and fixable crashes), thus new tarballs and shipping kdelibs 4.13.2, workspace 4.11.12 (depending on kdelibs 4.13.2) and kde-runtime 4.13.2 does not sound very reasonable to me. I expected that we would just tag 4.12.0 and so on from the 4.11 branch. Whether it's master or branch doesn't really matter. I agree that this is something we learned from kdelibs that we need to keep the releases going even if they do not contain new features. Cheers Martin signature.asc Description: This is a digitally signed message part.
Re: Review Request 112236: krunner: Add the full name of completion matches to history
On Aug. 24, 2013, 12:08 p.m., Thomas Lübking wrote: kwin/clients/aurorae/themes/plastik/package/contents/ui/main.qml, line 274 http://git.reviewboard.kde.org/r/112236/diff/1/?file=184315#file184315line274 ... ;-) Harald Hvaal wrote: yeah, not sure what happened with post-review here, but it's definitely not part of the actual commit :) post-review happily includes any changes you have in a tree. Always do git stash before using it ;-) - Martin --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112236/#review38466 --- On Aug. 24, 2013, 11:52 a.m., Harald Hvaal wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112236/ --- (Updated Aug. 24, 2013, 11:52 a.m.) Review request for kde-workspace and Plasma. Description --- Previously you would not record the actual item you chose after choosing an auto-completion, so if you typed say lal and you chose a completion way down the list, the history item would not reflect that choice, only what you typed to get to the completion list. This commit will fill the history combo box with the actual hit you executed, instead of half-complete strings that don't make sense until you actually select them. Exact matches are added as-is. example: type ass, and you get Qt Assistant. Instead of adding ass to the history, Qt Assistant will be added. Diffs - krunner/interfaces/default/interface.cpp 505e0aa6c02233fba0ff7ae9ce1133e8c7542104 kwin/clients/aurorae/themes/plastik/package/contents/ui/main.qml 8832d1d03e47a4e6382877d18ee664ecd4d12343 Diff: http://git.reviewboard.kde.org/r/112236/diff/ Testing --- Thanks, Harald Hvaal
Re: Review Request 112235: Move focus to search field upon typing from result list
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112235/#review38471 --- what about cursor up/down keys? You probably want to stay in the results list when using those. - Martin Gräßlin On Aug. 24, 2013, 11:45 a.m., Harald Hvaal wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112235/ --- (Updated Aug. 24, 2013, 11:45 a.m.) Review request for kde-workspace. Description --- Upon typing after having the focus moved down to the results, the focus will be moved back to the search input immediately. Diffs - krunner/interfaces/default/resultitem.cpp 31fe94c96195fdaeaf548207f99b294f7d768203 krunner/interfaces/default/resultscene.cpp 514c2c82d0fe3c3aa04200b5e254a6489bffd89d Diff: http://git.reviewboard.kde.org/r/112235/diff/ Testing --- Thanks, Harald Hvaal
Re: Review Request 112328: Add case sensitive sorting option to KGlobalSettings and KDirSortFilterProxyModel
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112328/#review39355 --- KGlobalSettings moved into KDE4Support in frameworks 5. I would suggest to not add new API to an already deprecated class. Maybe you want to provide a patch for the proper way in frameworks? - Martin Gräßlin On Aug. 29, 2013, 3:11 p.m., Eugene Shalygin wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112328/ --- (Updated Aug. 29, 2013, 3:11 p.m.) Review request for Dolphin and kdelibs. Description --- There are number of people who want to have case sensitive sorting in file manager and file dialogs (that would be consistent with locale settings). The proposed patch adds a property to KGlobalSettings (caseSensitiveSorting()) and the new signal sortingModeChanged(), which deprecates naturalSortingChanged(), so the clients can connect to only one event and do not resort their models twice (maybe and option for the event to pass the change reason would be useful?). KDirSortFilterProxyModel is changed in a trivial manner to use new event and handle case sensitivity changes. This addresses bug https://bugs.kde.org/show_bug.cgi?id=148550. http://bugs.kde.org/show_bug.cgi?id=https://bugs.kde.org/show_bug.cgi?id=148550 Diffs - kdeui/kernel/kglobalsettings.h 96da20e kdeui/kernel/kglobalsettings.cpp 2e3a7eb kfile/kdirsortfilterproxymodel.h 63cf04c kfile/kdirsortfilterproxymodel.cpp 34ddcd2 Diff: http://git.reviewboard.kde.org/r/112328/diff/ Testing --- Manual Thanks, Eugene Shalygin
Re: kde-workspace master becomes Qt5-based
On Thursday 19 September 2013 19:30:52 Sebastian Kügler wrote: Hi all, We're planning to merge the frameworks-scratch branch of kde-workspace into master next Monday. That means if you're building your Plasma yourself from Git (and you're not yet ready for Plasma2 ;)), you should switch to the KDE/4.11 branch. The build will fail otherwise, so you will notice. This was a public service announcement. (And you can ask questions here.) Follow-up: frameworks-scratch got just merged into master. Please do not push into the frameworks-scratch branch any more! Cheers Martin signature.asc Description: This is a digitally signed message part.
Re: Re: kde-workspace master becomes Qt5-based
On Tuesday 24 September 2013 13:51:57 Albert Astals Cid wrote: El Dimarts, 24 de setembre de 2013, a les 11:34:45, Martin Gräßlin va escriure: On Thursday 19 September 2013 19:30:52 Sebastian Kügler wrote: Hi all, We're planning to merge the frameworks-scratch branch of kde-workspace into master next Monday. That means if you're building your Plasma yourself from Git (and you're not yet ready for Plasma2 ;)), you should switch to the KDE/4.11 branch. The build will fail otherwise, so you will notice. This was a public service announcement. (And you can ask questions here.) Follow-up: frameworks-scratch got just merged into master. Please do not push into the frameworks-scratch branch any more! Can this be blocked more efficiently in the git repo instead of relying on people to do the right thing? Ben already did that. Which means kudos to our awesome sysadmins! Cheers Martin signature.asc Description: This is a digitally signed message part.
Re: Review Request 112991: Fix compilation rules of KDE-Workspace under OSX/Macports
On Oct. 17, 2013, 5:11 p.m., Gilles Caulier wrote: Ok, thanks for you feedback. I make patch with git/master, not KDE/4.11 branch. I will checkout this code and adapt patch accordingly Gilles Caulier I make patch with git/master, not KDE/4.11 branch You are aware that master is Qt5 based and will not result in a 4.12 release? If your aim is to fix it for the next release KDE/4.11 is the better branch, for master the situation is different anyway as our basic assumption unix == X11 is no longer valid. Also it's probably too early to put any non-linux adjustments into master - the chances that it breaks is rather high given that only Linux developers are working on it right now. - Martin --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112991/#review41897 --- On Sept. 29, 2013, 7:15 p.m., Gilles Caulier wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112991/ --- (Updated Sept. 29, 2013, 7:15 p.m.) Review request for kde-workspace. Bugs: https://trac.macports.org/ticket/33780 http://bugs.kde.org/show_bug.cgi?id=https://trac.macports.org/ticket/33780 Repository: kde-workspace Description --- This patch fix broken compilation under OSX / macports about kde-workspace. Patch do not touch implementation. Only compilation rules are changed in cmake script to follow the way way than Windows rules, where no X11 lib are available. By this way, Oxygen is compiled and installed to macport and digiKam has a suitable GUI under OSX. See my Macports bug report for details : https://trac.macports.org/ticket/33780 Gilles Caulier Diffs - CMakeLists.txt c37ab8b kcontrol/CMakeLists.txt a25aaa0 libs/CMakeLists.txt 9d71a03 Diff: http://git.reviewboard.kde.org/r/112991/diff/ Testing --- I tested this patch under my macbook pro, using a fresh install of Macports (KDE 4.11.1 / Qt 4.8.5) As kde-workspace macports package is broken, i checkout code from KDE git/master repository and fixed compilation rules as well. Thanks, Gilles Caulier
Re: Review Request 113415: Port KSplashQML to Qt5 + remove XLib stuff + replace x11eventFilter with DBus interface (Wayland++)
On Oct. 24, 2013, 6:17 p.m., Fredrik Höglund wrote: What's the cold startup time like for KSplashQML compared to KSplashX? Let's not forget that the reason KPlash was rewritten to only depend on X + libjpeg + libpng was so that the startup time would be limited by the time it takes to load the images for the theme. but that's a long time ago, isn't it? So Moore's Law... Anyway could be interesting to do such comparisons on spinning metal. - Martin --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113415/#review42299 --- On Oct. 24, 2013, 4:19 p.m., Martin Klapetek wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113415/ --- (Updated Oct. 24, 2013, 4:19 p.m.) Review request for kde-workspace and KDE Frameworks. Repository: kde-workspace Description --- Ports KSplashQML to Qt5 and strips any XLib stuff. Martin G. suggested to try if it would simply work without XLib/XCB bits at all and it does work in testing mode, I'm unsure if it also works after actual login, but I don't see why it wouldn't. KSplash was also receiving the stage messages via XAtoms, which I replaced with simple DBus interface. That means that all the parts in the login sequence need to be updated (I'll do it) to emit dbus signal instead of sending X message. I used the same API as the XAtoms were using, but let me know if you think this could be changed. In some future I'd probably also change the stages resolution as it's not too robust and does not help paralelization much. I have a plan in my head but that's for another commit. I'd also like to remove the old KSplashX and port its default theme to QML and provide together with KSplashQML as Classic or KDE 4 theme. Diffs - ksplash/ksplashqml/CMakeLists.txt 8cd5305 ksplash/ksplashqml/SplashApp.h 3c55be9 ksplash/ksplashqml/SplashApp.cpp 7c528d0 ksplash/ksplashqml/SplashWindow.h 9680c1e ksplash/ksplashqml/SplashWindow.cpp 4417643 ksplash/ksplashqml/SystemInfo.h 7a74554 ksplash/ksplashqml/main.cpp c2722ab9 ksplash/ksplashqml/org.kde.KSplash.xml PRE-CREATION ksplash/ksplashqml/themes/Minimalistic/main.qml e4fb8b8 Diff: http://git.reviewboard.kde.org/r/113415/diff/ Testing --- Theme successfully loads, displays fullscreen (Plasma panels are not covered, not sure why), all stages passes, then it terminates itself. Thanks, Martin Klapetek
Re: kde-workspace git broken ?
On Monday 11 November 2013 13:29:32 Hugo Pereira Da Costa wrote: Hello, I think commits 9f70241d57f3ba1013b9f28650478c8bbb1233e0 137dd285bdf821fd2c8a5c17e30dc9c1a6eca87b 09ea308ab55505efe7aeaebcd4aef6292cd884e6 seriously broke kde-workspace yes, it's broken. I already created a sysadmin request. Unfortunately a similar commit got just pushed to KDE/4.11. So at the moment both master and KDE/4.11 branch are broken. Please remember: do not push a revert of a merge commit. Cheers Martin signature.asc Description: This is a digitally signed message part.
Re: Review Request 114219: Do not encode QString to QByteArray and cast it back to QString. This causes problem when there are Unicode characters in ${HOME}
: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/114219/ --- (Updated Nov. 30, 2013, 5:38 a.m.) Review request for kde-workspace, David Faure, Martin Gräßlin, and Hugo Pereira Da Costa. Bugs: 327919 http://bugs.kde.org/show_bug.cgi?id=327919 Repository: kde-workspace Description --- list.join already returns a QString and there is no need to encode it and cast back to QString again P.S. for a patch that applies to both KDE4 and KF5(master for kde-workspace, frameworks for kdelibs?) How should I submit review request? Should I add both in branch or submit two review request? (But often the patch cannot apply directly due to context or file path changes). Diffs - kcontrol/krdb/krdb.cpp 92d84e9 Diff: http://git.reviewboard.kde.org/r/114219/diff/ Testing --- Compiles. Fixes the problem here. Thanks, Yichao Yu
Re: Review Request 114841: Screenlocker: don't set the mouse cursor when grabbing the mouse
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114841/#review46960 --- how does that behave with the normal locker (that is no screensaver)? - Martin Gräßlin On Jan. 5, 2014, 9:55 a.m., Wolfgang Bauer wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114841/ --- (Updated Jan. 5, 2014, 9:55 a.m.) Review request for kde-workspace and Martin Gräßlin. Bugs: 311571 and 316459 http://bugs.kde.org/show_bug.cgi?id=311571 http://bugs.kde.org/show_bug.cgi?id=316459 Repository: kde-workspace Description --- Setting the cursor to ArrowCursor when calling XGrabPointer() prevents the Screen savers from blanking the mouse cursor. I don't know why this has been done in the first place, but I couldn't see any negative effect by setting it to None. Now the mouse cursor even changes to the IBeam again when over the password field, which I find more intuitive. Diffs - ksmserver/screenlocker/ksldapp.cpp f0526cf Diff: https://git.reviewboard.kde.org/r/114841/diff/ Testing --- Configure a Screen saver in systemsettings and wait for it to kick in (or lock the screen manually). Previously (since 4.10) the mouse cursor stayed visible, now it is blanked like it was the case before 4.10. Moving the mouse/pressing a key (to quit the Screen saver) makes the mouse cursor appear again as it should, regardless of whether the screen is locked or not. Thanks, Wolfgang Bauer
Re: Review Request 114841: Screenlocker: don't set the mouse cursor when grabbing the mouse
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114841/#review47020 --- Ship it! If you have the possibility (build setup) please merge to master and fix the merge conflict I expect to see :-) I merged 4.11 into master yesterday so there should no be anything else which could conflict. - Martin Gräßlin On Jan. 5, 2014, 9:55 a.m., Wolfgang Bauer wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114841/ --- (Updated Jan. 5, 2014, 9:55 a.m.) Review request for kde-workspace and Martin Gräßlin. Bugs: 311571 and 316459 http://bugs.kde.org/show_bug.cgi?id=311571 http://bugs.kde.org/show_bug.cgi?id=316459 Repository: kde-workspace Description --- Setting the cursor to ArrowCursor when calling XGrabPointer() prevents the Screen savers from blanking the mouse cursor. I don't know why this has been done in the first place, but I couldn't see any negative effect by setting it to None. Now the mouse cursor even changes to the IBeam again when over the password field, which I find more intuitive. Diffs - ksmserver/screenlocker/ksldapp.cpp f0526cf Diff: https://git.reviewboard.kde.org/r/114841/diff/ Testing --- Configure a Screen saver in systemsettings and wait for it to kick in (or lock the screen manually). Previously (since 4.10) the mouse cursor stayed visible, now it is blanked like it was the case before 4.10. Moving the mouse/pressing a key (to quit the Screen saver) makes the mouse cursor appear again as it should, regardless of whether the screen is locked or not. Thanks, Wolfgang Bauer
Re: Review Request 114841: Screenlocker: don't set the mouse cursor when grabbing the mouse
On Jan. 8, 2014, 8:10 a.m., Martin Gräßlin wrote: If you have the possibility (build setup) please merge to master and fix the merge conflict I expect to see :-) I merged 4.11 into master yesterday so there should no be anything else which could conflict. Wolfgang Bauer wrote: I have committed to 4.11, but I don't have a KF5/PW2 build setup at the moment. I just merged it to master and pushed as http://commits.kde.org/kde-workspace/98768f680480df64a60fbe1802ca8ee05fb28887 - Martin --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114841/#review47020 --- On Jan. 8, 2014, 9:59 a.m., Wolfgang Bauer wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114841/ --- (Updated Jan. 8, 2014, 9:59 a.m.) Review request for kde-workspace and Martin Gräßlin. Bugs: 311571 and 316459 http://bugs.kde.org/show_bug.cgi?id=311571 http://bugs.kde.org/show_bug.cgi?id=316459 Repository: kde-workspace Description --- Setting the cursor to ArrowCursor when calling XGrabPointer() prevents the Screen savers from blanking the mouse cursor. I don't know why this has been done in the first place, but I couldn't see any negative effect by setting it to None. Now the mouse cursor even changes to the IBeam again when over the password field, which I find more intuitive. Diffs - ksmserver/screenlocker/ksldapp.cpp f0526cf Diff: https://git.reviewboard.kde.org/r/114841/diff/ Testing --- Configure a Screen saver in systemsettings and wait for it to kick in (or lock the screen manually). Previously (since 4.10) the mouse cursor stayed visible, now it is blanked like it was the case before 4.10. Moving the mouse/pressing a key (to quit the Screen saver) makes the mouse cursor appear again as it should, regardless of whether the screen is locked or not. Thanks, Wolfgang Bauer
Re: Review Request 110813: Add support for just smooth window movement and resizing to wobbly windows effect
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/110813/#review47043 --- sorry for the late review. It didn't end in my review board inbox :-( I think it's conceptually wrong to have a no wobble option in an effect which is called wobbly windows. If one doesn't want wobbly windows one can just disable the effect. I understand what you want to achieve but adding it here makes the effect significantly more complex and adds a very obscure feature to the effect. I doubt that any users would find this feature and thus it would be a solution just for your private use case. We try to not go this road. I thank you for this contribution and hope that you understand my reasoning why I don't want this in wobbly windows. Please excuse again the long time for the review. - Martin Gräßlin On June 3, 2013, 9:55 p.m., Stefanos Harhalakis wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/110813/ --- (Updated June 3, 2013, 9:55 p.m.) Review request for kde-workspace. Bugs: 319887 http://bugs.kde.org/show_bug.cgi?id=319887 Repository: kde-workspace Description --- A patch that adds the ability to the wobbly windows effect not to wobble the window (i.e. not change its shape). This results in two things: 1) Have a very smooth movement of windows that can be customized 2) Have a nice effect when resizing a window (is resizing is enabled) It does this by adding the noWobble attribute to the effect and then using that attribute to disable a subset of the functionality. It also adds an additional wobbliness level: shifts levels 0-4 to 1-5 and makes 0 the level without wobbliness. Diffs - kwin/effects/wobblywindows/wobblywindows.h 69d1f87 kwin/effects/wobblywindows/wobblywindows.cpp ad1842c kwin/effects/wobblywindows/wobblywindows_config.ui 0498a64 Diff: https://git.reviewboard.kde.org/r/110813/diff/ Testing --- I'm running this for the past day without issues. Thanks, Stefanos Harhalakis
Re: Review Request 115001: add kf5 namespace to kglobalaccel dbus interface
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115001/#review47339 --- Wouldn't that break KDElibs4 applications talking to kglobalacceld from KF5? - Martin Gräßlin On Jan. 13, 2014, 4:51 p.m., Jonathan Riddell wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115001/ --- (Updated Jan. 13, 2014, 4:51 p.m.) Review request for kde-workspace and Martin Klapetek. Repository: kde-workspace Description --- add kf5 namespace to kglobalaccel dbus interface to prevent files and runtime interfaces overlapping with kdelibs 4 Goes with these reviews for kf5 and kde-runtime https://git.reviewboard.kde.org/r/114999/ https://git.reviewboard.kde.org/r/115000/ Diffs - kcontrol/keys/CMakeLists.txt 072e614 kcontrol/keys/kglobalshortcutseditor.cpp ca11fcd Diff: https://git.reviewboard.kde.org/r/115001/diff/ Testing --- Thanks, Jonathan Riddell
Re: Review Request 114999: Add KF5 namespace to dbus interface
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114999/#review47390 --- like with the other patch I'm not sure whether that's a good idea as that breaks any communication with the kdelibs4. - Martin Gräßlin On Jan. 14, 2014, 4:21 p.m., Jonathan Riddell wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114999/ --- (Updated Jan. 14, 2014, 4:21 p.m.) Review request for KDE Runtime, David Faure and Martin Klapetek. Repository: kglobalaccel Description --- Rename the dbus interfaces from org.kde.kglobalaccel.Component to org.kde.kf5kglobalaccel.Component and org.kde.kglobalaccel to org.kde.kf5kglobalaccel this prevents files overlapping with equivalents in kdelibs4 and prevents an overlap of the dbus interface Diffs - src/CMakeLists.txt d48e92e src/kglobalaccel.h d11881c src/kglobalaccel.cpp 54d18ec src/kglobalaccel_p.h b1528dc src/kglobalshortcutinfo.h 7f0e18a src/org.kde.KF5KGlobalAccel.xml PRE-CREATION src/org.kde.KGlobalAccel.xml 8746551 src/org.kde.kf5kglobalaccel.Component.xml PRE-CREATION src/org.kde.kglobalaccel.Component.xml ec21201 Diff: https://git.reviewboard.kde.org/r/114999/diff/ Testing --- Thanks, Jonathan Riddell
Re: Review Request 114999: Add KF5 namespace to dbus interface
On Jan. 14, 2014, 8:22 p.m., Martin Gräßlin wrote: like with the other patch I'm not sure whether that's a good idea as that breaks any communication with the kdelibs4. Thomas Lübking wrote: is there a functional incompitibility between SC4 and KF5 that requires such disambiguation or is this merely a package installation conflict resolution? I did the port of kglobalacceld and had no problem at all. Also running KWin/5 linked against kglobalaccel talking to kglobalacceld/4 was no problem. So from that I *assume* that there are no functional incompatibilities. - Martin --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114999/#review47390 --- On Jan. 14, 2014, 4:21 p.m., Jonathan Riddell wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114999/ --- (Updated Jan. 14, 2014, 4:21 p.m.) Review request for KDE Runtime, David Faure and Martin Klapetek. Repository: kglobalaccel Description --- Rename the dbus interfaces from org.kde.kglobalaccel.Component to org.kde.kf5kglobalaccel.Component and org.kde.kglobalaccel to org.kde.kf5kglobalaccel this prevents files overlapping with equivalents in kdelibs4 and prevents an overlap of the dbus interface Diffs - src/CMakeLists.txt d48e92e src/kglobalaccel.h d11881c src/kglobalaccel.cpp 54d18ec src/kglobalaccel_p.h b1528dc src/kglobalshortcutinfo.h 7f0e18a src/org.kde.KF5KGlobalAccel.xml PRE-CREATION src/org.kde.KGlobalAccel.xml 8746551 src/org.kde.kf5kglobalaccel.Component.xml PRE-CREATION src/org.kde.kglobalaccel.Component.xml ec21201 Diff: https://git.reviewboard.kde.org/r/114999/diff/ Testing --- Thanks, Jonathan Riddell
Re: Qt 5.3 to log all debug/warning/error messages to journald on systemd systems
On Monday 20 January 2014 14:40:17 Thiago Macieira wrote: See subject. We're trying to decide whether we should enable journald by default on Linux distributions that carry it. If we do, it means any application that is not launched from a terminal would automatically write to journald instead. KDE applications are the largest users of qDebug today. If we changed the default, it would mean ~/.xsession-errors would probably become rather empty. Is that ok for KDE? I like this +1 -- Martin Gräßlin signature.asc Description: This is a digitally signed message part.
Re: Review Request 115079: don't install dbus interface files in kglobalaccel
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115079/#review47868 --- I guess it's obvious from the matching review request for kgloballacel: I consider copying the files to every user as the wrong solution. In Qt terms I would give it a -2. - Martin Gräßlin On Jan. 21, 2014, 1:48 p.m., Jonathan Riddell wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115079/ --- (Updated Jan. 21, 2014, 1:48 p.m.) Review request for kde-workspace and Martin Gräßlin. Repository: kde-workspace Description --- to go with https://git.reviewboard.kde.org/r/115078/ use local dbus interface files Diffs - kcontrol/keys/org.kde.kglobalaccel.Component.xml PRE-CREATION kcontrol/keys/org.kde.KGlobalAccel.xml PRE-CREATION kcontrol/keys/CMakeLists.txt 072e614 Diff: https://git.reviewboard.kde.org/r/115079/diff/ Testing --- Thanks, Jonathan Riddell
Re: Review Request 115079: don't install dbus interface files in kglobalaccel
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115079/#review47882 --- much better! Thanks for working on it. - Martin Gräßlin On Jan. 21, 2014, 3:51 p.m., Jonathan Riddell wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115079/ --- (Updated Jan. 21, 2014, 3:51 p.m.) Review request for kde-workspace and Martin Gräßlin. Repository: kde-workspace Description --- to go with https://git.reviewboard.kde.org/r/115078/ use local dbus interface files Diffs - kcontrol/keys/CMakeLists.txt 072e614 Diff: https://git.reviewboard.kde.org/r/115079/diff/ Testing --- Thanks, Jonathan Riddell
Re: Re: Splitting kde-workspace and kde-runtime proposal
On Tuesday 21 January 2014 13:12:58 David Hubner wrote: In the plasma sprint we have done a session to plan what we are going to do with kde-workspace/kde-runtime repositories, here is the proposal we came with. We are going to create 2 new repos called plasma-desktop and plasma-workspace, we decided to use plasma as a prefix so in the future we can have more workspaces and desktops without being in the awkward situation of having one wrongly labeled as KDE while others are not (thinking on for example having Razorqt/lxde as part of KDE in the future). Current kde-workspace and kde- runtime will be kept for history reasons. plasma-desktop will hold all specific pieces tied to the desktop experience, for example the touchpadenabler only makes sense on laptops. plasma-workspace will contain all bits that are generic enough to be of interest for more than one form factor, for example we need appmenu in both, tablet and desktop. Additionally we want to split optional dependencies (kinfocenter for example) and other projects that are quite big. Some of the new repositories we want to create are: powerdevil kwin oxygen (containing a full Oxygen experience) ksysguard kinfocenter klipper... A full list of the proposed new repositories can be found at [1]. Finally some other bits could be merged with some Frameworks, it is the case for example of solid-hardware and solid-networkstatus that should be moved to solid. Again, this is a proposal so please! send any feedback you might have. Cheers. Hi, I am ok with splitting KInfocenter into it's own repo. It might also be an idea to split System Settings kcm's and KInfocenter kcm's into a repo called KCM or something and then have KInfocenter and System Settings repo's just for the app. That might be an idea. Or throw together system settings, kinfocenter and all the KCMs. But that are details for when we do the split :-) I feel I might be missing some history here though.. why is this being done? The idea to split workspace into more repos had been around for quite some time. We noticed that we have applications with very different scope in kde- workspace. Historically the workspace has been X11 only and everything in the repo should only make sense when running the KDE workspaces. That's not really the case any more: * we have applications which are built on all platforms (e.g. KInfoCenter) * we have X11 only applications which are used outside the KDE workspaces (e.g. KWin) * we have applications which are only used on one or the other shell of the kde-workspaces * we have libraries other applications depend on and we don't want applications to depend on the workspaces (famous example kdevelop depending on ksysguard) Just look at the CMakeList and the mess we do with finding the requirements for e.g. KWin. Let's first find some random X11 library to check whether we are on X11 and then turn the optional X11 dependency into a mandatory and start to find more required stuff. That's IMHO already a good enough technical reason to do the split. There are more reasons to split up and clean up. I just wanted to outline one rather obvious. Cheers Martin signature.asc Description: This is a digitally signed message part.
Re: Review Request 115311: [kwin] Don't call into GL without a context
On Jan. 28, 2014, 8:03 a.m., Commit Hook wrote: This review has been submitted with commit 77202a2102306a288431462e6c874216ac4cd29c by Martin Gräßlin on behalf of James Jones to branch KDE/4.11. James Jones wrote: Thanks Martin. Will the change automatically be merged up to master as well, or should I submit a separate patch there? I already merged it to master directly after the push. Thanks for the patch! - Martin --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115311/#review48446 --- On Jan. 28, 2014, 8:03 a.m., James Jones wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115311/ --- (Updated Jan. 28, 2014, 8:03 a.m.) Review request for kde-workspace, Thomas Lübking and Marco Martin. Repository: kde-workspace Description --- [kwin] Don't call into GL without a context After losing current from the EGL or GLX context, calls to the GL or GLES functions have undefined behavior. Perform all cleanup that may touch OpenGL and check for GL errors before losing current from the context. Diffs - kwin/egl_wayland_backend.cpp b229cdd84161a64d5cd93c189514067867773e7f kwin/eglonxbackend.cpp dd41da55b94821802f2d1464794db39bd636088a kwin/glxbackend.cpp 73f463e9df43c2cd71836ce3f48da84fb4df35ed kwin/scene_opengl.cpp 961e81fbcc39940bc49179899e034ad8a9e802cd Diff: https://git.reviewboard.kde.org/r/115311/diff/ Testing --- Compiled/Installed kde-workspace on x86 kwin_gles (EGL+X11) - Tested mode switching kwin (GLX) - Tested mode switching File Attachments git-format-patch version of patch https://git.reviewboard.kde.org/media/uploaded/files/2014/01/27/11681ec3-81a1-4a02-a165-6e442f4e21f5__0001-kwin-Don-t-call-into-GL-without-a-context.patch Thanks, James Jones
Re: Review Request 115515: [oxygen] Check whether we are on platform X11 before calling into xcb
On Feb. 6, 2014, 12:26 p.m., Hugo Pereira Da Costa wrote: @Martin in kstyles/oxygen you are missing oxygenblurhelper (and likely kate will crash when showing a tooltip) in kwin/clients/oxygen (but might be another review) oxygenclient oxygensizegrip config/oxygendetectwidget Other than that, ship it ! kwin/clients doesn't matter (yet) ;-) - Martin --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115515/#review49102 --- On Feb. 6, 2014, 11:55 a.m., Martin Gräßlin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115515/ --- (Updated Feb. 6, 2014, 11:55 a.m.) Review request for kde-workspace and Hugo Pereira Da Costa. Repository: kde-workspace Description --- [oxygen] Check whether we are on platform X11 before calling into xcb Just because we compiled with X11 present doesn't mean we run on X11. This fixes quite a lot of crashers when trying to run framework apps on Wayland. @Hugo: do you know of further files which use xcb unconditionally and which I just haven't hit yet? Diffs - kstyles/oxygen/CMakeLists.txt ca9cc1a1710187f5013482ef502c456238fd4373 kstyles/oxygen/oxygenshadowhelper.cpp f77093daa4f907afd55333032c6f6b618ad2f47f kstyles/oxygen/oxygenstylehelper.cpp 7e2de41822bf0205e2cec7ea82c8e3f6751a2a6b kstyles/oxygen/oxygenwindowmanager.h 7ef54d3172b0b0a92b6c33152858cb923b9d3f1e kstyles/oxygen/oxygenwindowmanager.cpp 308ce4d049b6ce4c9c2cdd67448d351747f34b19 libs/oxygen/oxygenhelper.h 8d734e6e3f3a23c0257cc400417c340ba6d48bea libs/oxygen/oxygenhelper.cpp 1fe78c24c053e74f2179de00ee9a5701997a0acb Diff: https://git.reviewboard.kde.org/r/115515/diff/ Testing --- running Kate on Wayland till it crashes (or doesn't) Thanks, Martin Gräßlin
Re: Review Request 115515: [oxygen] Check whether we are on platform X11 before calling into xcb
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115515/ --- (Updated Feb. 6, 2014, 1:22 p.m.) Review request for kde-workspace and Hugo Pereira Da Costa. Changes --- added test to blurhelper. It hadn't crashed as createAtom was adjusted to return 0. Repository: kde-workspace Description --- [oxygen] Check whether we are on platform X11 before calling into xcb Just because we compiled with X11 present doesn't mean we run on X11. This fixes quite a lot of crashers when trying to run framework apps on Wayland. @Hugo: do you know of further files which use xcb unconditionally and which I just haven't hit yet? Diffs (updated) - kstyles/oxygen/CMakeLists.txt ca9cc1a1710187f5013482ef502c456238fd4373 kstyles/oxygen/oxygenblurhelper.cpp d774b2c45f8ec452311d34f35adf4d9bcc39693d kstyles/oxygen/oxygenshadowhelper.cpp f77093daa4f907afd55333032c6f6b618ad2f47f kstyles/oxygen/oxygenstylehelper.cpp 7e2de41822bf0205e2cec7ea82c8e3f6751a2a6b kstyles/oxygen/oxygenwindowmanager.h 7ef54d3172b0b0a92b6c33152858cb923b9d3f1e kstyles/oxygen/oxygenwindowmanager.cpp 308ce4d049b6ce4c9c2cdd67448d351747f34b19 libs/oxygen/oxygenhelper.h 8d734e6e3f3a23c0257cc400417c340ba6d48bea libs/oxygen/oxygenhelper.cpp 1fe78c24c053e74f2179de00ee9a5701997a0acb Diff: https://git.reviewboard.kde.org/r/115515/diff/ Testing --- running Kate on Wayland till it crashes (or doesn't) Thanks, Martin Gräßlin
Review Request 115519: Do not use KDE_VERSION_STRING for workspace applications
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115519/ --- Review request for kde-workspace and Release Team. Repository: kde-workspace Description --- Workspace is no longer synced with kdelibs releases. Thus if compiled against e.g. 4.12 we want our workspace apps to still be 4.11. Diffs - kwin/clients/oxygen/demo/main.cpp 83d9d83 kwrited/kwrited.cpp cab2d6b plasma/desktop/shell/main.cpp ea3d43d startkde.cmake 30d5ab5 statusnotifierwatcher/statusnotifierwatcher.cpp 97ae164 systemsettings/app/main.cpp 78109e0 kstyles/oxygen/config/main.cpp 5a5286e kstyles/oxygen/demo/main.cpp 005395b ksysguard/gui/ksysguard.cpp 2ad34f2 config-workspace.h.cmake b3ba37d khotkeys/kcm_hotkeys/kcm_hotkeys.cpp 389a361 kinfocenter/main.cpp c28f7cf krunner/main.cpp 6eac316 kscreensaver/kblank_screensaver/blankscrn.cpp 99ef649 kscreensaver/krandom_screensaver/random.cpp 4047184 Diff: https://git.reviewboard.kde.org/r/115519/diff/ Testing --- compiled with 4.12.60 kdelibs and looked at e.g. startkde and systemsettings which have now 4.11.6. Thanks, Martin Gräßlin
Re: Review Request 115515: [oxygen] Check whether we are on platform X11 before calling into xcb
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115515/ --- (Updated Feb. 7, 2014, 12:13 p.m.) Status -- This change has been marked as submitted. Review request for kde-workspace and Hugo Pereira Da Costa. Repository: kde-workspace Description --- [oxygen] Check whether we are on platform X11 before calling into xcb Just because we compiled with X11 present doesn't mean we run on X11. This fixes quite a lot of crashers when trying to run framework apps on Wayland. @Hugo: do you know of further files which use xcb unconditionally and which I just haven't hit yet? Diffs - kstyles/oxygen/CMakeLists.txt ca9cc1a1710187f5013482ef502c456238fd4373 kstyles/oxygen/oxygenblurhelper.cpp d774b2c45f8ec452311d34f35adf4d9bcc39693d kstyles/oxygen/oxygenshadowhelper.cpp f77093daa4f907afd55333032c6f6b618ad2f47f kstyles/oxygen/oxygenstylehelper.cpp 7e2de41822bf0205e2cec7ea82c8e3f6751a2a6b kstyles/oxygen/oxygenwindowmanager.h 7ef54d3172b0b0a92b6c33152858cb923b9d3f1e kstyles/oxygen/oxygenwindowmanager.cpp 308ce4d049b6ce4c9c2cdd67448d351747f34b19 libs/oxygen/oxygenhelper.h 8d734e6e3f3a23c0257cc400417c340ba6d48bea libs/oxygen/oxygenhelper.cpp 1fe78c24c053e74f2179de00ee9a5701997a0acb Diff: https://git.reviewboard.kde.org/r/115515/diff/ Testing --- running Kate on Wayland till it crashes (or doesn't) Thanks, Martin Gräßlin
Re: Review Request 116600: depend upon CMake 2.8.12
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116600/#review51906 --- Ship it! Ship It! - Martin Gräßlin On March 4, 2014, 5:25 p.m., Bhushan Shah wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116600/ --- (Updated March 4, 2014, 5:25 p.m.) Review request for kde-workspace and Martin Gräßlin. Repository: kde-workspace Description --- FindXCB requires cmake 2.8.12 and kde-workspace asks for 2.8.6 so this warning/error(?) is printed CMake Warning (dev) at /opt/kf5/share/ECM/find-modules/FindXCB.cmake:64 (message): Your project should require at least CMake 2.8.12 to use FindXCB.cmake Diffs - CMakeLists.txt 41923b6 Diff: https://git.reviewboard.kde.org/r/116600/diff/ Testing --- works, warning is gone Thanks, Bhushan Shah
Re: Review Request 108808: Do not reset opacity if call for GetWindowProperty fails
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/108808/ --- (Updated March 18, 2014, 4:28 p.m.) Status -- This change has been discarded. Review request for kdelibs and kwin. Repository: kdelibs Description --- Do not reset opacity if call for GetWindowProperty fails The opacity got unconditionally reset to the default value of fully opaque. In case the property has a value of 0 and the call to get the window property fails, the window would be shown again. This issue was spotted with colibri which faded out it's window and got a flash to fully opaque again. I'm sorry that it's difficult to read this patch properly due to the existing mixing of tabs and whitespaces. The patch changes two things: * only updates the value if all checks succeed * initialise the data_ret variable to not cause a crash on free BUG: 314427 FIXED-IN: 4.10.1 Diffs - kdeui/windowmanagement/netwm.cpp cf28339f90dff1d17ed17842c7c11d8a9718a459 Diff: https://git.reviewboard.kde.org/r/108808/diff/ Testing --- Thanks, Martin Gräßlin
Re: Review Request 116956: rename kglobalaccel to kglobalaccel5 for co-installability
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116956/#review53930 --- what about just renaming the installed targets? I don't see a benefit in renaming the desktop files, etc. in the source dir. Otherwise we will have to rename again with kglobalaccel6 - Martin Gräßlin On March 21, 2014, 5:35 p.m., Jonathan Riddell wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116956/ --- (Updated March 21, 2014, 5:35 p.m.) Review request for KDE Runtime, Plasma and Martin Gräßlin. Repository: kde-runtime Description --- kde-runtime will soon get an alpha release. Because both KF5 and kdelibs4 applications should be able to be installed and run it should be co-installable with kde-runtime from KDE 4 times. Starting at the top of the cmake file I've renamed kglobalaccel to see if it's sane to do so. Diffs - kglobalaccel/org.kde.kglobalaccel.service.in d8576b0 kglobalaccel/CMakeLists.txt 8bc8bea kglobalaccel/kglobalaccel.desktop a61516e kglobalaccel/kglobalaccel.notifyrc 9e3ecd3 kglobalaccel/kglobalaccel5.desktop PRE-CREATION kglobalaccel/kglobalaccel5.notifyrc PRE-CREATION kglobalaccel/main.cpp d788b64 Diff: https://git.reviewboard.kde.org/r/116956/diff/ Testing --- Thanks, Jonathan Riddell
Re: Review Request 116956: rename kglobalaccel to kglobalaccel5 for co-installability
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116956/#review54152 --- Ship it! Ship It! - Martin Gräßlin On March 25, 2014, 7:44 p.m., Jonathan Riddell wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116956/ --- (Updated March 25, 2014, 7:44 p.m.) Review request for KDE Runtime, Plasma and Martin Gräßlin. Repository: kde-runtime Description --- kde-runtime will soon get an alpha release. Because both KF5 and kdelibs4 applications should be able to be installed and run it should be co-installable with kde-runtime from KDE 4 times. Starting at the top of the cmake file I've renamed kglobalaccel to see if it's sane to do so. Diffs - kglobalaccel/CMakeLists.txt e0b739f kglobalaccel/kglobalaccel.desktop a61516e kglobalaccel/org.kde.kglobalaccel.service.in d8576b0 Diff: https://git.reviewboard.kde.org/r/116956/diff/ Testing --- Thanks, Jonathan Riddell
Re: small but Big request
On Friday 28 March 2014 16:17:19 David Boosalis wrote: At the risk of getting 50 lashes. Can I make a request for new KDE Git build instructions. All the instructions for building might be out there and I know there is the uber kdesrc-build script, but you really got to squint to get all the details just right. Spring has yet to arrive in Canada, and I thought it would be nice to try and build KDE5 from Git this weekend. There might be others like me that want to try this exercise. I assume with KDE5 you mean all the frameworks and maybe Plasma 2? For the frameworks there are build instructions: http://community.kde.org/Frameworks/Building It's written around kdesrc-build and I can only recommend to use that tool and don't try to build it manually. Just having the dependency resolutions between all the frameworks is rather complex. So use the tooling :-) The nice thing about using kdesrc-build is that you easily get also Plasma 2 and the frameworks based applications. Cheers Martin signature.asc Description: This is a digitally signed message part.
Re: Review Request 117157: Unlock session via DBus
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117157/#review54538 --- I also have problems imagining what a use case for this is and I consider this as a security issue. It basically means that the session can get unlocked without going through authentication. - Martin Gräßlin On March 29, 2014, 12:58 p.m., Kirill Elagin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117157/ --- (Updated March 29, 2014, 12:58 p.m.) Review request for kde-workspace. Bugs: 314989 http://bugs.kde.org/show_bug.cgi?id=314989 Repository: kde-workspace Description --- Unlock session via DBus Make org.freedesktop.ScreenSaver.SetActive(false) unlock session. Diffs - plasma-workspace/ksmserver/screenlocker/interface.cpp ecb30a37b1a207cf9dab8c53b1b879108a99a45b plasma-workspace/ksmserver/screenlocker/ksldapp.h b292b62f4df073fff31bcbfd0e39f4c4fe04c92d plasma-workspace/ksmserver/screenlocker/ksldapp.cpp f2e5262524447e8ae1df1fbf6543297c3be3e6b8 Diff: https://git.reviewboard.kde.org/r/117157/diff/ Testing --- I've tested this with KDE 4.11.5 which I'm currently running. Rebasing to master was completely trivial; I've looked through the code and I believe all the assumptions I made are still valid in master. Thanks, Kirill Elagin
Re: Review Request 117157: Unlock session via DBus
On March 29, 2014, 1:05 p.m., Martin Gräßlin wrote: I also have problems imagining what a use case for this is and I consider this as a security issue. It basically means that the session can get unlocked without going through authentication. Kirill Elagin wrote: You have to authenticate anyway to access the DBus session bus. and running applications? It would allow $evilsecretservice to unlock the screen when $agent needs to use the system after remote installing some software. Since Snowden this doesn't sound so far fetched any more (unfortunately). - Martin --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117157/#review54538 --- On March 29, 2014, 12:58 p.m., Kirill Elagin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117157/ --- (Updated March 29, 2014, 12:58 p.m.) Review request for kde-workspace. Bugs: 314989 http://bugs.kde.org/show_bug.cgi?id=314989 Repository: kde-workspace Description --- Unlock session via DBus Make org.freedesktop.ScreenSaver.SetActive(false) unlock session. Diffs - plasma-workspace/ksmserver/screenlocker/interface.cpp ecb30a37b1a207cf9dab8c53b1b879108a99a45b plasma-workspace/ksmserver/screenlocker/ksldapp.h b292b62f4df073fff31bcbfd0e39f4c4fe04c92d plasma-workspace/ksmserver/screenlocker/ksldapp.cpp f2e5262524447e8ae1df1fbf6543297c3be3e6b8 Diff: https://git.reviewboard.kde.org/r/117157/diff/ Testing --- I've tested this with KDE 4.11.5 which I'm currently running. Rebasing to master was completely trivial; I've looked through the code and I believe all the assumptions I made are still valid in master. Thanks, Kirill Elagin
Re: Review Request 117157: Unlock session via DBus
On March 29, 2014, 1:05 p.m., Martin Gräßlin wrote: I also have problems imagining what a use case for this is and I consider this as a security issue. It basically means that the session can get unlocked without going through authentication. Kirill Elagin wrote: You have to authenticate anyway to access the DBus session bus. Martin Gräßlin wrote: and running applications? It would allow $evilsecretservice to unlock the screen when $agent needs to use the system after remote installing some software. Since Snowden this doesn't sound so far fetched any more (unfortunately). Thomas Lübking wrote: you need access to a random shell of that user (what does not imply you actively logged into it), but can expose another session that pot. allows access to other logins (mail, webservices, even banking) this should (by default) no way be possible. any way to circumvent authentication to this very session should be guarded by a special knowwhatido key or require active authentication (ie. passing the pass hash via dbus - what's insecure enough by itself) Kirill Elagin wrote: If you've installed your evil software you already have a thousand of ways of accessing the system. My point is: if you've got privileges to issue this DBus call, you already have privileges to bypass the lockscreen. This is just a _sane_ way of doing so, because if you're an $evilagent you don't care how many lines of shell code it will take you to bypass the lockscreen, but if you are the user, it starts to matter. no, the lockscreen is secure. If you are logged in at a tty there is no way to unlock the screen - the only way to bypass the lock is to kill ksmserver which results in the session being killed. - Martin --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117157/#review54538 --- On March 29, 2014, 12:58 p.m., Kirill Elagin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117157/ --- (Updated March 29, 2014, 12:58 p.m.) Review request for kde-workspace. Bugs: 314989 http://bugs.kde.org/show_bug.cgi?id=314989 Repository: kde-workspace Description --- Unlock session via DBus Make org.freedesktop.ScreenSaver.SetActive(false) unlock session. Diffs - plasma-workspace/ksmserver/screenlocker/interface.cpp ecb30a37b1a207cf9dab8c53b1b879108a99a45b plasma-workspace/ksmserver/screenlocker/ksldapp.h b292b62f4df073fff31bcbfd0e39f4c4fe04c92d plasma-workspace/ksmserver/screenlocker/ksldapp.cpp f2e5262524447e8ae1df1fbf6543297c3be3e6b8 Diff: https://git.reviewboard.kde.org/r/117157/diff/ Testing --- I've tested this with KDE 4.11.5 which I'm currently running. Rebasing to master was completely trivial; I've looked through the code and I believe all the assumptions I made are still valid in master. Thanks, Kirill Elagin
Re: Review Request 117157: Unlock session via DBus
On March 29, 2014, 1:05 p.m., Martin Gräßlin wrote: I also have problems imagining what a use case for this is and I consider this as a security issue. It basically means that the session can get unlocked without going through authentication. Kirill Elagin wrote: You have to authenticate anyway to access the DBus session bus. Martin Gräßlin wrote: and running applications? It would allow $evilsecretservice to unlock the screen when $agent needs to use the system after remote installing some software. Since Snowden this doesn't sound so far fetched any more (unfortunately). Thomas Lübking wrote: you need access to a random shell of that user (what does not imply you actively logged into it), but can expose another session that pot. allows access to other logins (mail, webservices, even banking) this should (by default) no way be possible. any way to circumvent authentication to this very session should be guarded by a special knowwhatido key or require active authentication (ie. passing the pass hash via dbus - what's insecure enough by itself) Kirill Elagin wrote: If you've installed your evil software you already have a thousand of ways of accessing the system. My point is: if you've got privileges to issue this DBus call, you already have privileges to bypass the lockscreen. This is just a _sane_ way of doing so, because if you're an $evilagent you don't care how many lines of shell code it will take you to bypass the lockscreen, but if you are the user, it starts to matter. Martin Gräßlin wrote: no, the lockscreen is secure. If you are logged in at a tty there is no way to unlock the screen - the only way to bypass the lock is to kill ksmserver which results in the session being killed. Kirill Elagin wrote: It seems to me that it's not secure actually. If you have a look at the bug I mentioned, you'll see a one-liner that unlocks the screen, right? Unfortunately I'm not really familiar with KDE internals, but I don't see any way to avoid this. (I should point out that, still, I don't see this as a security issue; once an $evilagent got your shell, you already lost, because now he can _modify memory_ of any of your processes, including ksmserver.) But if you still don't agree… well, will it be acceptable to have an option in `kscreensaverrc` or `ksmserverrc` that triggers this behaviour? Thomas Lübking wrote: It seems to me that it's not secure actually. As pointed out in the very first comment, i consider this a serious bug and security issue - yes. FTR: There's a difference between the ability to poke memory on the one hand and have a nice GUI access to your money accounts, an open ssl session or root shell of a running system. Accessing random shells/client conections of the current session is the pot. privilegue escalation here, open to an attacker how could eg. not manipulate the software stack. you'll see a one-liner that unlocks the screen, right? this shouldn't be, and is clearly a bug. Will be the first thing I look into on Monday. will it be acceptable to have an option in `kscreensaverrc` or `ksmserverrc` that triggers this behaviour? That doesn't increase security. If you want such a functionality it must go into logind IMHO. - Martin --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117157/#review54538 --- On March 29, 2014, 12:58 p.m., Kirill Elagin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117157/ --- (Updated March 29, 2014, 12:58 p.m.) Review request for kde-workspace. Bugs: 314989 http://bugs.kde.org/show_bug.cgi?id=314989 Repository: kde-workspace Description --- Unlock session via DBus Make org.freedesktop.ScreenSaver.SetActive(false) unlock session. Diffs - plasma-workspace/ksmserver/screenlocker/interface.cpp ecb30a37b1a207cf9dab8c53b1b879108a99a45b plasma-workspace/ksmserver/screenlocker/ksldapp.h b292b62f4df073fff31bcbfd0e39f4c4fe04c92d plasma-workspace/ksmserver/screenlocker/ksldapp.cpp f2e5262524447e8ae1df1fbf6543297c3be3e6b8 Diff: https://git.reviewboard.kde.org/r/117157/diff/ Testing --- I've tested this with KDE 4.11.5 which I'm currently running. Rebasing to master was completely trivial; I've looked through the code and I believe all the assumptions I made are still valid in master. Thanks, Kirill Elagin
Re: Re: Review Request 117157: Unlock session via DBus
On Sunday 30 March 2014 18:06:52 Thiago Macieira wrote: Leaving access to an open shell is certainly bad enough - beyond question. The question is whether gaining direct access to a running session and random open clients (and leaving the stage untraced) is more valuable and thus worth pretection. We're assuming that the attacker already gained access to the session at this point. For example, if you've left a logged in shell in a virtual console. At that point, it's already game over. Since that is so, let's stop trying to cover the sun with a sieve. Instead, let's make the life of developers and helpgivers easier: let there be an unlock command via D-Bus, without transiting the password again. Personally I have to disagree. To me the graphical login is a an asset which needs to be protected in a stronger way. Access to a tty should not equal access to the graphical system. The fact that X is broken should not result in us adding further insecurities which need to be fixed up once we transit to Wayland. The opposite has to happen: all the small security issues we let in, because X was already broken need to get fixed. This thread turned into a nice TODO list :-) Our default should be to be secure and not to allow to be insecure because developers need to have an easy way to fix their setup. Btw. the greeter theme allows to be changed and the theme does not require authentication. It's up to the greeter theme to decide how to grant access. We even ship one theme for Plasma Active which does not provide any security. For use cases which require to allow quitting the locker through DBus this should be provided through the greeter theme, not through the lock process. If one wants to make the system less secure it should have to be explicitly changed and should require more privs. Cheers Martin signature.asc Description: This is a digitally signed message part.
Re: Review Request 117157: Unlock session via DBus
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117157/#review54849 --- Please see different approach in https://git.reviewboard.kde.org/r/117324/ to use logind as an authority to unlock. - Martin Gräßlin On March 29, 2014, 12:58 p.m., Kirill Elagin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117157/ --- (Updated March 29, 2014, 12:58 p.m.) Review request for kde-workspace. Bugs: 314989 http://bugs.kde.org/show_bug.cgi?id=314989 Repository: kde-workspace Description --- Unlock session via DBus Make org.freedesktop.ScreenSaver.SetActive(false) unlock session. Diffs - plasma-workspace/ksmserver/screenlocker/interface.cpp ecb30a37b1a207cf9dab8c53b1b879108a99a45b plasma-workspace/ksmserver/screenlocker/ksldapp.h b292b62f4df073fff31bcbfd0e39f4c4fe04c92d plasma-workspace/ksmserver/screenlocker/ksldapp.cpp f2e5262524447e8ae1df1fbf6543297c3be3e6b8 Diff: https://git.reviewboard.kde.org/r/117157/diff/ Testing --- I've tested this with KDE 4.11.5 which I'm currently running. Rebasing to master was completely trivial; I've looked through the code and I believe all the assumptions I made are still valid in master. Thanks, Kirill Elagin
Re: Freedesktop summit 2014
On Thursday 17 April 2014 20:17:38 David Faure wrote: I represented KDE again this year at the freedesktop summit in Nuremberg. Please find our report below. sorry to ask, but were there any discussions about the fact that GTK+ based applications start to be broken on other environments? See for example [1], especially screenshot at [2]. To me it feels very strange to have a mail on the great collaboration on the one day and then seeing that bug report the other day. It would mean a lot of work on our side to fix this and it's clearly a new feature which means we cannot fix in 4.11 time life. For me it's very questionable to consider any future collaboration if the GTK+ camp is deliberately (!) breaking integration on other environments. Cheers Martin [1] https://bugs.kde.org/show_bug.cgi?id=333554 [2] https://bug727414.bugzilla-attachments.gnome.org/attachment.cgi?id=273375 signature.asc Description: This is a digitally signed message part.
Re: Review Request 117644: screenlocker: don't leave behind screensaver processes
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117644/#review56240 --- would you please also adapt that for plasma-workspace repo (new master)? - Martin Gräßlin On April 22, 2014, 10:41 p.m., Wolfgang Bauer wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117644/ --- (Updated April 22, 2014, 10:41 p.m.) Review request for kde-workspace and Plasma. Bugs: 224200 http://bugs.kde.org/show_bug.cgi?id=224200 Repository: kde-workspace Description --- Currently the screen locker just kills the greeter (kscreenlocker_greet) when the screen is unlocked by the user during the grace time. But apparently this can leave behind running screensaver processes launched by the greeter, see the bug report (which has the highest number of votes of all open bugs AFAICS). This patch changes this to only terminate the greeter, and adds a signal handler to the greeter to exit gracefully in this case. The signal handler exits with return code 1, so that it is not possible to circumvent the password input by just sending a SIGTERM. (the screen locker restarts the greeter in case it doesn't quit with exit code 0) Diffs - ksmserver/screenlocker/greeter/main.cpp d898734 ksmserver/screenlocker/ksldapp.cpp 3dfcc9e Diff: https://git.reviewboard.kde.org/r/117644/diff/ Testing --- Configure a legacy screensaver in Systemsettings-Display and Monitor-Screen Locker, be sure to leave Require Password after disabled. Wait for the screen locker to kick in. Unlock the screen by moving the mouse or pressing a key. Check the process list. Without this patch at least kswarm.kss and kblankscreen.kss reliably kept running after unlocking the screen on my system. With this patch they quit themselves. I'm using this patch for over two weeks now, and I haven't seen any left-over screen saver processes any more (and I even set the timeout to 1 minute). I also tried to terminate kscreenlocker_greet manually by running killall kscreenlocker_greet from a text console in case of a password required, and the locker didn't quit, you still have to enter the password. Thanks, Wolfgang Bauer
Re: Review Request 117091: Force the screen locker's greeter to show the password input field in case of immediateLock
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117091/#review56241 --- Is that only relevant for the legacy (XSS) locker or also for the new locker? I'm just wondering whether it needs to be ported to master - Martin Gräßlin On April 22, 2014, 10:34 p.m., Wolfgang Bauer wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117091/ --- (Updated April 22, 2014, 10:34 p.m.) Review request for kde-workspace, Plasma and Aaron J. Seigo. Bugs: 327947 and 329076 http://bugs.kde.org/show_bug.cgi?id=327947 http://bugs.kde.org/show_bug.cgi?id=329076 Repository: kde-workspace Description --- If the screen locker is set to not require a password to unlock, it will not show the password input field even when the powermanagement settings suspend the system and are set to require a password after resume (when it was already running at that point). This locks people out of their system. This patch adds a signal handler for SIGUSR1 that switches the running greeter to immediateLock mode. The locker sends that signal to make sure the greeter shows the password input field when necessary. Diffs - ksmserver/screenlocker/greeter/greeterapp.cpp c5e2f85 ksmserver/screenlocker/greeter/main.cpp d898734 ksmserver/screenlocker/greeter/greeterapp.h 8b79188 ksmserver/screenlocker/ksldapp.cpp 3dfcc9e Diff: https://git.reviewboard.kde.org/r/117091/diff/ Testing --- Disable Require password after in the screen locker settings (the default), set it to start after 1 min. (for easier testing). Enable Suspend session after and set it to 2 minutes. (set the action to Suspend, Hibernate, or Lock Screen, doesn't matter) Make sure Lock screen on resume is enabled in the powermanagements Advanced Options (it is by default). After 1 minute the screen locker kicks in, and doesn't require a password. After 2 minutes the session gets suspended, hibernated or locked, and requires a password to resume. Without this patch no password dialog is shown, the user cannot resume the session by entering the password. With this patch this works: there is a password input field, the session is unlocked when the user enters the password. Thanks, Wolfgang Bauer
Re: Review Request 117644: screenlocker: don't leave behind screensaver processes
On April 23, 2014, 7:36 a.m., Martin Gräßlin wrote: would you please also adapt that for plasma-workspace repo (new master)? Wolfgang Bauer wrote: Yes, I will. Should I create a new review request for that, or should I just submit it? The code looks pretty straight forward to me, so I don't think it's really needed to have another review request. Unless... in master we have a few auto-tests, so perhaps you want to add a test for this case? Given the mentioned number of votes it might not be the worst idea to have a regression test for it ;-) - Martin --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117644/#review56240 --- On April 22, 2014, 10:41 p.m., Wolfgang Bauer wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117644/ --- (Updated April 22, 2014, 10:41 p.m.) Review request for kde-workspace and Plasma. Bugs: 224200 http://bugs.kde.org/show_bug.cgi?id=224200 Repository: kde-workspace Description --- Currently the screen locker just kills the greeter (kscreenlocker_greet) when the screen is unlocked by the user during the grace time. But apparently this can leave behind running screensaver processes launched by the greeter, see the bug report (which has the highest number of votes of all open bugs AFAICS). This patch changes this to only terminate the greeter, and adds a signal handler to the greeter to exit gracefully in this case. The signal handler exits with return code 1, so that it is not possible to circumvent the password input by just sending a SIGTERM. (the screen locker restarts the greeter in case it doesn't quit with exit code 0) Diffs - ksmserver/screenlocker/greeter/main.cpp d898734 ksmserver/screenlocker/ksldapp.cpp 3dfcc9e Diff: https://git.reviewboard.kde.org/r/117644/diff/ Testing --- Configure a legacy screensaver in Systemsettings-Display and Monitor-Screen Locker, be sure to leave Require Password after disabled. Wait for the screen locker to kick in. Unlock the screen by moving the mouse or pressing a key. Check the process list. Without this patch at least kswarm.kss and kblankscreen.kss reliably kept running after unlocking the screen on my system. With this patch they quit themselves. I'm using this patch for over two weeks now, and I haven't seen any left-over screen saver processes any more (and I even set the timeout to 1 minute). I also tried to terminate kscreenlocker_greet manually by running killall kscreenlocker_greet from a text console in case of a password required, and the locker didn't quit, you still have to enter the password. Thanks, Wolfgang Bauer
Re: Review Request 117091: Force the screen locker's greeter to show the password input field in case of immediateLock
On April 23, 2014, 7:41 a.m., Martin Gräßlin wrote: Is that only relevant for the legacy (XSS) locker or also for the new locker? I'm just wondering whether it needs to be ported to master Wolfgang Bauer wrote: Yes. I just tried, and the screen locker in master does have the same problem. I wasn't able yet to get powerdevil working with plasma next, but when I call the Lock() method via DBUS while the locker is already running and still in the grace period (I manually created a ~/.config/kscreensaverrc for that), the password input field is missing there as well, so you cannot unlock. Wolfgang Bauer wrote: I have adapted the patch to master as well: https://build.opensuse.org/package/view_file/home:wolfi323:branches:KDE:Unstable:Frameworks/plasma-workspace5/screenlocker-always-show-password-dialog-when-needed.patch?expand=0 I tested it and it fixes the problem there too. Should I create a new review request for this? There are no drastic differences to the KDE4 patch anyway. no I think it's fine to have it with one review request. For master I suggest to switch from NULL to nullptr, though. - Martin --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117091/#review56241 --- On April 22, 2014, 10:34 p.m., Wolfgang Bauer wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117091/ --- (Updated April 22, 2014, 10:34 p.m.) Review request for kde-workspace, Plasma and Aaron J. Seigo. Bugs: 327947 and 329076 http://bugs.kde.org/show_bug.cgi?id=327947 http://bugs.kde.org/show_bug.cgi?id=329076 Repository: kde-workspace Description --- If the screen locker is set to not require a password to unlock, it will not show the password input field even when the powermanagement settings suspend the system and are set to require a password after resume (when it was already running at that point). This locks people out of their system. This patch adds a signal handler for SIGUSR1 that switches the running greeter to immediateLock mode. The locker sends that signal to make sure the greeter shows the password input field when necessary. Diffs - ksmserver/screenlocker/greeter/greeterapp.cpp c5e2f85 ksmserver/screenlocker/greeter/main.cpp d898734 ksmserver/screenlocker/greeter/greeterapp.h 8b79188 ksmserver/screenlocker/ksldapp.cpp 3dfcc9e Diff: https://git.reviewboard.kde.org/r/117091/diff/ Testing --- Disable Require password after in the screen locker settings (the default), set it to start after 1 min. (for easier testing). Enable Suspend session after and set it to 2 minutes. (set the action to Suspend, Hibernate, or Lock Screen, doesn't matter) Make sure Lock screen on resume is enabled in the powermanagements Advanced Options (it is by default). After 1 minute the screen locker kicks in, and doesn't require a password. After 2 minutes the session gets suspended, hibernated or locked, and requires a password to resume. Without this patch no password dialog is shown, the user cannot resume the session by entering the password. With this patch this works: there is a password input field, the session is unlocked when the user enters the password. Thanks, Wolfgang Bauer
Re: Review Request 117779: fix crash when textureNode-texture() is null
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117779/#review56587 --- I'd rather not want to see us hide this crash. There is an underlying problem which needs a more proper fix. I recently hit this problem myself on one on my systems and a crash one can reproduce is as good as fixed ;-) - Martin Gräßlin On April 26, 2014, 12:51 a.m., Alexander Richardson wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117779/ --- (Updated April 26, 2014, 12:51 a.m.) Review request for kde-workspace and Plasma. Repository: plasma-framework Description --- fix crash when textureNode-texture() is null I get this crash very frequently on my system. This is probably only fixing the symptom and not the underlying issue, however at least plasma no longer crashes every few minutes Diffs - src/declarativeimports/core/windowthumbnail.cpp 59255f75994adb96b30fb503c759b2e9110ab708 Diff: https://git.reviewboard.kde.org/r/117779/diff/ Testing --- No longer crashes Thanks, Alexander Richardson
Re: Review Request 117779: fix crash when textureNode-texture() is null
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117779/#review56707 --- did you notice any pattern which triggers the crash? That could help me to reproduce and find the root cause. - Martin Gräßlin On April 26, 2014, 12:51 a.m., Alexander Richardson wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117779/ --- (Updated April 26, 2014, 12:51 a.m.) Review request for kde-workspace and Plasma. Repository: plasma-framework Description --- fix crash when textureNode-texture() is null I get this crash very frequently on my system. This is probably only fixing the symptom and not the underlying issue, however at least plasma no longer crashes every few minutes Diffs - src/declarativeimports/core/windowthumbnail.cpp 59255f75994adb96b30fb503c759b2e9110ab708 Diff: https://git.reviewboard.kde.org/r/117779/diff/ Testing --- No longer crashes Thanks, Alexander Richardson
Re: Review Request 117779: fix crash when textureNode-texture() is null
On April 28, 2014, 10:24 a.m., Martin Gräßlin wrote: did you notice any pattern which triggers the crash? That could help me to reproduce and find the root cause. I found the condition myself: 1. show thumbnail 2. let it hide again 3. show thumbnail for same task when switching to another task crash does not happen. - Martin --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117779/#review56707 --- On April 26, 2014, 12:51 a.m., Alexander Richardson wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117779/ --- (Updated April 26, 2014, 12:51 a.m.) Review request for kde-workspace and Plasma. Repository: plasma-framework Description --- fix crash when textureNode-texture() is null I get this crash very frequently on my system. This is probably only fixing the symptom and not the underlying issue, however at least plasma no longer crashes every few minutes Diffs - src/declarativeimports/core/windowthumbnail.cpp 59255f75994adb96b30fb503c759b2e9110ab708 Diff: https://git.reviewboard.kde.org/r/117779/diff/ Testing --- No longer crashes Thanks, Alexander Richardson
Compatibility problems with latest GTK+ applications
Hi all, I need some advice. The new GTK+ release introduced and enforces client-side- decorations (CSD) and that is causing severe compatibility problems with Plasma Workspaces (especially the stable release which we cannot adjust any more). I'll give a list of issues below. I'm not sure what we can do about it. I think we need to do something about it, because this looks like a road back to GTK apps can only be used in GNOME Shell resulting in KDE apps can only be used in Plasma. Because of that I think the compatibility problems matters to everyone also our application developers. I think the right thing is to contact the GTK+ developers and point out the problems, but I am probably the wrong person for it as I'm a known opponent of CSD and fear that my feedback would be ignored. As a fallback strategy we could enforce server-side-decorations (SSD) for all GTK+ applications which would look strange [4] but at least no loss in functionality. Of course there are things we can do in KWin to improve compatibility, but 4.11 is feature frozen and these would require new code and we have certainly more important things to do than to fix client breakage (we don't do that in general). It would be way easier if GTK+ would detect whether the environment supports CSD and use a normal application design if that's not supported. This would not only be relevant for us, but especially for tiling window managers. List of issues I noticed: * A hung window can no longer be closed or moved. Technical explanation: there is a ping protocol to detect hung applications. KWin only sends ping requests when the window is being closed from the window manager (e.g. decoration close button or Alt+F4). * quick tiling broken, see [1]. It includes the shadow in calculation. * black shadows around window if compositing gets disabled after the window got opened [2]. Technical explanation: GTK+ doesn't detect that compositing gets enabled/disabled. Other direction is broken, too. * double borders if window gets opened when compositing is disabled [3]. Technical explanation: GTK+ uses the Motif Window Manager Hints to tell how the decoration should look like. KWin is not the Motif Window Manager and does not fully support the hints. Fun fact our code has a comment that we follow Metacity to only support those hints which are not stupid. * double border if decorations on KWin side are enforced [4] (Alt+F3 - More Actions - No Border). Technical explanation: GTK+ doesn't detect the re- parenting and doesn't hide it's own deco and shadow * Wrong resize cursor is shown on edges [6] * Windows don't have a maximize and minimize button although configured in the decoration settings * Incorrect drag delay: KWin uses either a drag delay on distance or on time for starting move or resize. GTK+ only has a drag delay on distance for moving and none for resizing. That's an important accessibility feature to have that globally configurable * Windows can no longer be shaded * Windows can no longer be put into window tabs * Dialogs don't have a close button [5] (unless run without compositing) * Ignores the configured mouse actions, uses (hard coded?) actions. Defaulting to lower window on middle click while we use window tab drag on middle click * no visual distinction between active and inactive application * Broken window snapping: window snaps to shadow [7] * Mismatching application window menu [8]. Note our menu could be invoked through DBus. I consider this as a big problem as that's the only way to e.g. add windows to activities (which is also broken now) or to access the advanced window manager features GNOME doesn't know of. That list I figured out in less than 10 min usage of a new GTK+ application. I assume and fear that the list is much longer. Any advice on how to handle this situation is appreciated. Cheers Martin [1] https://picasaweb.google.com/lh/photo/_q8BS4WN91ZvjEGSsiUkqtMTjNZETYmyPJy0liipFm0?feat=directlink [2] https://picasaweb.google.com/lh/photo/hGjW0RLjGF3UVw5wFnPMqdMTjNZETYmyPJy0liipFm0?feat=directlink [3] https://picasaweb.google.com/lh/photo/hriAb8OI1Yh8BtxtAg5bG9MTjNZETYmyPJy0liipFm0?feat=directlink [4] https://picasaweb.google.com/lh/photo/9LLTICdSJhcHl-SCBWc_i9MTjNZETYmyPJy0liipFm0?feat=directlink [5] https://picasaweb.google.com/lh/photo/-krx0xvGFjq9TwkEC4Bh_tMTjNZETYmyPJy0liipFm0?feat=directlink [6] https://picasaweb.google.com/lh/photo/BfuvOcm0_v_BG-SkoxXJ29MTjNZETYmyPJy0liipFm0?feat=directlink [7] https://picasaweb.google.com/lh/photo/Vya50RqaVq1ij6sMDHEnbdMTjNZETYmyPJy0liipFm0?feat=directlink [8] https://picasaweb.google.com/lh/photo/2Xa5xNqDmjGL-bXwFzmPWdMTjNZETYmyPJy0liipFm0?feat=directlink signature.asc Description: This is a digitally signed message part.
Re: Re: Compatibility problems with latest GTK+ applications
On Wednesday 07 May 2014 11:24:03 Marco Martin wrote: On Wednesday 07 May 2014 10:11:37 Martin Gräßlin wrote: * A hung window can no longer be closed or moved. Technical explanation: there is a ping protocol to detect hung applications. KWin only sends ping requests when the window is being closed from the window manager (e.g. decoration close button or Alt+F4). * quick tiling broken, see [1]. It includes the shadow in calculation. Is this due to the motif hints? no, it's due to the shadow being part of the window. * Windows can no longer be put into window tabs * Dialogs don't have a close button [5] (unless run without compositing) I think it's their HIG to not have buttons in dialogs whatever, ours doesn't :-) Cheers Martin signature.asc Description: This is a digitally signed message part.
Re: Compatibility problems with latest GTK+ applications
Martin GräßlinOn Wednesday 07 May 2014 10:11:37 wrote: Any advice on how to handle this situation is appreciated. As several people responded that I should report the issues I just did that and reported the following bug reports against GTK: * CSD styled windows don't react on compositing changes [1] * Double decorated windows [2] * CSD styled windows do not detect when re-parented to a decoration [3] * CSD context menu ignores _NET_ALLOWED_ACTIONS [4] * CSD context menu doesn't use _NET_DESKTOP_NAMES [5] * CSD context menu doesn't honor _NET_DESKTOP_LAYOUT [6] * Shadow included in CSD window [7] * Window disappears when middle clicking client side decoration [8] * Missing maximize and minimize buttons in client side decoration [9] * Decoration buttons do not follow custom specified layout in desktop environment [10] * A hung GTK application cannot be closed [11] * Context menu on window decoration is not the one of the environment [12] * No time based drag delay on window moving [13] * No drag delay on window resize [14] Cheers Martin [1] https://bugzilla.gnome.org/show_bug.cgi?id=729767 [2] https://bugzilla.gnome.org/show_bug.cgi?id=729769 [3] https://bugzilla.gnome.org/show_bug.cgi?id=729772 [4] https://bugzilla.gnome.org/show_bug.cgi?id=729773 [5] https://bugzilla.gnome.org/show_bug.cgi?id=729777 [6] https://bugzilla.gnome.org/show_bug.cgi?id=729780 [7] https://bugzilla.gnome.org/show_bug.cgi?id=729781 [8] https://bugzilla.gnome.org/show_bug.cgi?id=729782 [9] https://bugzilla.gnome.org/show_bug.cgi?id=729783 [10] https://bugzilla.gnome.org/show_bug.cgi?id=729784 [11] https://bugzilla.gnome.org/show_bug.cgi?id=729786 [12] https://bugzilla.gnome.org/show_bug.cgi?id=729788 [13] https://bugzilla.gnome.org/show_bug.cgi?id=729792 [14] https://bugzilla.gnome.org/show_bug.cgi?id=729793 signature.asc Description: This is a digitally signed message part.
Re: Re: Compatibility problems with latest GTK+ applications
On Thursday 08 May 2014 14:39:49 Matthias Klumpp wrote: 2014-05-08 9:31 GMT+02:00 Martin Gräßlin mgraess...@kde.org: Martin GräßlinOn Wednesday 07 May 2014 10:11:37 wrote: Any advice on how to handle this situation is appreciated. As several people responded that I should report the issues I just did that and reported the following bug reports against GTK: * CSD styled windows don't react on compositing changes [1] * Double decorated windows [2] * CSD styled windows do not detect when re-parented to a decoration [3] * CSD context menu ignores _NET_ALLOWED_ACTIONS [4] * CSD context menu doesn't use _NET_DESKTOP_NAMES [5] * CSD context menu doesn't honor _NET_DESKTOP_LAYOUT [6] * Shadow included in CSD window [7] * Window disappears when middle clicking client side decoration [8] * Missing maximize and minimize buttons in client side decoration [9] * Decoration buttons do not follow custom specified layout in desktop environment [10] * A hung GTK application cannot be closed [11] * Context menu on window decoration is not the one of the environment [12] * No time based drag delay on window moving [13] * No drag delay on window resize [14] That is pretty great, thank you for taking the time! Some of these things unfortunately are design-decisions by GNOME, which I raised in IRC discussions a while ago, and where I think the interest is pretty low for fixing them (after getting some feedback on e.g. the CSD menus). which of the bug reports do you consider affected by design-decisions? I did not even report the mismatching cursors and the missing close button for dialogs and similar things. Cheers Martin signature.asc Description: This is a digitally signed message part.
Re: Re: Compatibility problems with latest GTK+ applications
On Friday 09 May 2014 09:54:53 John Layt wrote: On 8 May 2014 13:56, Sebastian Kügler se...@kde.org wrote: On Thursday, May 08, 2014 14:39:49 Matthias Klumpp wrote: However, to support the cross-desktop efforts, the GNOME people should maybe make a few compromises (e.g. make GTK+ behave differently on other DEs), especially since GTK+ is not just for GNOME but also used by other projects. This is actually at the root of all these problems. The GTK developers have at some point decided to couple the toolkit with the GNOME desktop, so GTK is -- in their view -- the toolkit for GNOME. It seems, the developers lost interest in keeping their cross-platform and cross-desktop promise. This is visible in other areas as well, such as theming: Theming APIs have been unstable and changed will-nilly for some time now, and the revolt of theme creators has calmed down by now (I suppose they all just walked away). I suppose this CSD episode will just speed up this exodus. We might be looking at a completely different GTK here than we did a few years ago, and that's pretty bad news for interoperability on the freedesktop. Exactly, they seem to have forgotten what the G actually stands for :-) Which makes me wonder how apps like Gimp who use Gtk but are not part of Gnome and want to be cross-desktop and cross-platform are going to be affected? And how are the other Gtk desktops like XFCE affected? XFCE is affected in that way that GTK developers opened bug reports against XFCE that their window manager is broken (stating it's the only one not supporting that, well KWin neither). Pardon my ignorance, but does Gtk impose CSD on all apps, or just those apps that opt-in to using it? I'm assuming it's opt-in, in which case perhaps the app authors don't realise the implication for other desktops/platforms? If they do realise what it means then that's their decision to only support Gnome and I don't see why we should even bother to try work around it: if it looks ugly elsewhere and that costs them users, that's their problem not ours. I'm not sure whether it's opt-in. Using the gtk3-demo and the gallery one can find many examples which enforce client-side-decorations. E.g. the standard message box dialog example. Either way, it seems to me that the Gtk app and desktop authors may be the people in the best position to influence Gtk to work on these issues, the Gtk maintainers may not care about what KDE thinks, but you'd hope that they would listen to their own users. Perhaps if/when the direct approach fails, we need to raise bugs reports against the apps themselves to get their authors interested? John. P.S. If Gtk keep this up, we could see another surge in apps switching to Qt, and that presents a great opportunity for KDE as a community interested in being cross-platform/desktop signature.asc Description: This is a digitally signed message part.
Re: Re: Compatibility problems with latest GTK+ applications
On Friday 09 May 2014 09:42:50 Daniel Nicoletti wrote: +Martin, does clicking on the shadow drawn by the window also prevents you from say focusing the window below (when no windeco is in place)? As from what I understood there is no hint margins. Maybe add this to your big list :P Just tried and of course that's broken, as well as focus follows mouse in that area. It's part of the bug that shadows are part of the window, but yes could be reported as an independent issue. signature.asc Description: This is a digitally signed message part.
Re: Review Request 118091: Changed formatting to fit the KDE Coding Standard
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/118091/#review57743 --- changing the coding style would completely break the already started port to xcb and thus Qt5, see https://git.reviewboard.kde.org/r/114178/ - Martin Gräßlin On May 12, 2014, 3:36 a.m., Uzair Shamim wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/118091/ --- (Updated May 12, 2014, 3:36 a.m.) Review request for KDE Base Apps. Repository: ksnapshot Description --- Certain parts of the code did not follow the kde code guideline and while I know it is not mandatory to follow the guide lines, but I felt I should fix it. There were certain parts I was unsure about so I left them as they were. Most of the changes are just with brace placement. KDE Coding Standard: http://techbase.kde.org/Policies/Kdelibs_Coding_Style Diffs - kbackgroundsnapshot.cpp 9d81ba1 kipiimagecollectionselector.h 10afedc kipiimagecollectionselector.cpp 5fe09d8 kipiinterface.h 1c18039 ksnapshot.cpp 0d8e996 ksnapshotimagecollectionshared.cpp 8ef1b0e ksnapshotinfoshared.cpp 0e0d840 ksnapshotobject.cpp 608f7dc ksnapshotpreview.cpp db4fd10 main.cpp 4b4f097 regiongrabber.cpp fb038a3 snapshottimer.cpp 843f06d windowgrabber.cpp 21a9531 Diff: https://git.reviewboard.kde.org/r/118091/diff/ Testing --- Compiles fine, seems to take screenshots just like before. As I said, 99% of the changes are just brace placement. Thanks, Uzair Shamim
Re: Review Request 118366: Porting keyboard module to Framework5
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118366/#review58618
---
kcms/keyboard/flags.cpp
https://git.reviewboard.kde.org/r/118366/#comment40807
why not removed?
kcms/keyboard/kcm_keyboard.ui
https://git.reviewboard.kde.org/r/118366/#comment40808
the rename looks unrelated
kcms/keyboard/kcmmisc.cpp
https://git.reviewboard.kde.org/r/118366/#comment40809
coding style: whitespace after for and in front of {
kcms/keyboard/kcmmisc.cpp
https://git.reviewboard.kde.org/r/118366/#comment40810
for new connects I would use the new compile time checked syntax.
kcms/keyboard/kcmmisc.cpp
https://git.reviewboard.kde.org/r/118366/#comment40811
why this commented debug statement?
kcms/keyboard/keyboard_config.cpp
https://git.reviewboard.kde.org/r/118366/#comment40812
remove?
kcms/keyboard/layouts_menu.cpp
https://git.reviewboard.kde.org/r/118366/#comment40813
remove?
kcms/keyboard/xinput_helper.h
https://git.reviewboard.kde.org/r/118366/#comment40814
I think it would have been better to keep the parent argument and pass it
to the base QObject
kcms/keyboard/xinput_helper.cpp
https://git.reviewboard.kde.org/r/118366/#comment40815
removing the event doesn't remove the fact that this method is not yet
ported. See the #if 0 which indicates that this code has not been adjusted to
xcb yet.
- Martin Gräßlin
On May 27, 2014, 11:02 p.m., shivam makkar wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118366/
---
(Updated May 27, 2014, 11:02 p.m.)
Review request for kde-workspace, KDE Frameworks and Andriy Rysin.
Repository: plasma-desktop
Description
---
Removed deprecated statements and ported keyboard module to framework 5.
Diffs
-
kcms/keyboard/bindings.cpp 21541e0
kcms/keyboard/flags.cpp b768586
kcms/keyboard/kcm_keyboard.ui 0062d1c
kcms/keyboard/kcm_keyboard_widget.cpp 21685eb
kcms/keyboard/kcmmisc.h 411bdd2
kcms/keyboard/kcmmisc.cpp 6f787ea
kcms/keyboard/kcmmiscwidget.ui 37fbaf4
kcms/keyboard/keyboard_config.cpp f3ff97c
kcms/keyboard/keyboard_daemon.cpp 25673b0
kcms/keyboard/keyboard_hardware.cpp dca49b6
kcms/keyboard/layout_memory.cpp 9e72361
kcms/keyboard/layout_memory_persister.cpp 8a6118a
kcms/keyboard/layouts_menu.cpp fd436c4
kcms/keyboard/xinput_helper.h 343d7ed
kcms/keyboard/xinput_helper.cpp b311579
Diff: https://git.reviewboard.kde.org/r/118366/diff/
Testing
---
Thanks,
shivam makkar
Re: Review Request 118563: kscreenlocker_greet: use SA_RESTART for signal handler
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/118563/#review59329 --- Ship it! the master (plasma-workspace repo) has a unit test for the signals. But I'm not sure whether we can extend it to catch this condition (we don't have a BSD CI system yet). - Martin Gräßlin On June 5, 2014, 3:16 p.m., Wolfgang Bauer wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/118563/ --- (Updated June 5, 2014, 3:16 p.m.) Review request for kde-workspace. Repository: kde-workspace Description --- As discussed in https://git.reviewboard.kde.org/r/117091/. Not using the SA_RESTART flag might (in theory) cause the greeter to be aborted (because certain syscalls may be interrupted and fail with EINTR). SA_RESTART seems to be the BSD default and is used by legacy signal() by default in glibc 2 and later as well, anyway. Diffs - ksmserver/screenlocker/greeter/main.cpp 4cac94c Diff: https://git.reviewboard.kde.org/r/118563/diff/ Testing --- I'm using this myself for one month without any problems. Thanks, Wolfgang Bauer
Re: Review Request 118763: Remove find_package(XCB) call as it is handled already by the top-level cmake file
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118763/#review60162
---
looks good to me, +1. Please add Hugo Pereira Da Costa to the Review Request,
though.
The review request made me wonder whether we still need to find XLib in Oxygen,
though. The parts shown only use XCB, so maybe we could just go for finding
only XCB?
- Martin Gräßlin
On June 15, 2014, 2:47 p.m., Bernd Steinhauser wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118763/
---
(Updated June 15, 2014, 2:47 p.m.)
Review request for kde-workspace.
Repository: oxygen
Description
---
No idea if kde-workspace is still the right group, if not, please change.
find_package(XCB) is called without specifying the required components. This
leads to linking to unused dependencies in case they are installed.
Since XCB is searched for in the top level cmake file in the repository,
there is no need to search for it again. The component required there (only
base XCB) is sufficient.
Although, this should be sufficient to fix the deps problem, it makes sense
to link to XCB::XCB instead of ${XCB_LIBRARIES}, since the former is what is
actually needed.
Diffs
-
kstyle/CMakeLists.txt 165b62a
liboxygen/CMakeLists.txt 0d1dd94
Diff: https://git.reviewboard.kde.org/r/118763/diff/
Testing
---
Thanks,
Bernd Steinhauser
Re: Review Request 118366: Porting keyboard module to Framework5
On May 28, 2014, 8:10 a.m., Martin Gräßlin wrote: kcms/keyboard/kcmmisc.cpp, lines 77-78 https://git.reviewboard.kde.org/r/118366/diff/2/?file=275630#file275630line77 for new connects I would use the new compile time checked syntax. shivam makkar wrote: I tried it but it was giving some error unable to deduce function pointer FUNC1 for which one was that? ::changed exists I think as a signal and a slot. You might need to cast it to the appropriate type. - Martin --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/118366/#review58618 --- On June 19, 2014, 8:48 p.m., shivam makkar wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/118366/ --- (Updated June 19, 2014, 8:48 p.m.) Review request for kde-workspace, KDE Frameworks and Andriy Rysin. Repository: plasma-desktop Description --- Removed deprecated statements and ported keyboard module to framework 5. Diffs - kcms/keyboard/bindings.cpp 21541e0 kcms/keyboard/bindings.cpp 21541e0 kcms/keyboard/flags.cpp b768586 kcms/keyboard/flags.cpp b768586 kcms/keyboard/flags.cpp 6d25443 kcms/keyboard/flags.cpp 3fb98e5 kcms/keyboard/kcm_keyboard.ui 0062d1c kcms/keyboard/kcm_keyboard.ui 0062d1c kcms/keyboard/kcm_keyboard_widget.cpp 21685eb kcms/keyboard/kcm_keyboard_widget.cpp 21685eb kcms/keyboard/kcmmisc.h 411bdd2 kcms/keyboard/kcmmisc.h 411bdd2 kcms/keyboard/kcmmisc.cpp 6f787ea kcms/keyboard/kcmmisc.cpp 6f787ea kcms/keyboard/kcmmisc.cpp d14ac2e kcms/keyboard/kcmmiscwidget.ui 37fbaf4 kcms/keyboard/kcmmiscwidget.ui 37fbaf4 kcms/keyboard/keyboard_config.cpp f3ff97c kcms/keyboard/keyboard_config.cpp f3ff97c kcms/keyboard/keyboard_config.cpp 49f059c kcms/keyboard/keyboard_daemon.cpp 25673b0 kcms/keyboard/keyboard_daemon.cpp 25673b0 kcms/keyboard/keyboard_hardware.cpp dca49b6 kcms/keyboard/keyboard_hardware.cpp dca49b6 kcms/keyboard/layout_memory.cpp 9e72361 kcms/keyboard/layout_memory.cpp 9e72361 kcms/keyboard/layout_memory_persister.cpp 8a6118a kcms/keyboard/layout_memory_persister.cpp 8a6118a kcms/keyboard/layouts_menu.cpp fd436c4 kcms/keyboard/layouts_menu.cpp fd436c4 kcms/keyboard/layouts_menu.cpp e357c6a kcms/keyboard/x11_helper.h 719b13f kcms/keyboard/x11_helper.cpp 0e2806e kcms/keyboard/xinput_helper.h 343d7ed kcms/keyboard/xinput_helper.h 343d7ed kcms/keyboard/xinput_helper.cpp b311579 kcms/keyboard/xinput_helper.cpp b311579 kcms/keyboard/xinput_helper.cpp b245e91 kcms/keyboard/xinput_helper.cpp 980338e Diff: https://git.reviewboard.kde.org/r/118366/diff/ Testing --- Thanks, shivam makkar
Re: Review Request 118898: KGamma: Apply user setting at login/startup
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118898/#review60809
---
kcmkgamma/init_kgamma.cpp
https://git.reviewboard.kde.org/r/118898/#comment42380
please use a copyright header as in
http://techbase.kde.org/Policies/Licensing_Policy#GPL_Header
kcmkgamma/init_kgamma.cpp
https://git.reviewboard.kde.org/r/118898/#comment42381
why delete config? I would just use a KSharedConfig::openConfig
- Martin Gräßlin
On June 23, 2014, 3:06 p.m., Wolfgang Bauer wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118898/
---
(Updated June 23, 2014, 3:06 p.m.)
Review request for kde-workspace, Plasma and Marcel Wiesweg.
Bugs: 218668
http://bugs.kde.org/show_bug.cgi?id=218668
Repository: kgamma
Description
---
KGamma's saved user settings are not applied on startup/login. The user has
to enter the KCM to apply them.
This makes it rather useless, as not even saving the settings system-wide
really works any more. (this requires an xorg.conf which normally doesn't
exist nowadays)
This patch factors out the function init_kgamma() into its own source file
(init_kgamma.cpp), adds a small executable (applykgammasettings) that just
applies those settings by calling that function, and installs
applykgammasettings.desktop to ${AUTOSTART_INSTALL_DIR} that runs
applykgammasettings on login.
PS: As there seems to be no kgamma group and this is desktop-related, I
decided to add the kde-workspace and plasma groups for review. I hope that's
ok... ;)
Diffs
-
kcmkgamma/CMakeLists.txt 3980023
kcmkgamma/applykgammasettings.cpp PRE-CREATION
kcmkgamma/applykgammasettings.desktop PRE-CREATION
kcmkgamma/init_kgamma.cpp PRE-CREATION
kcmkgamma/kgamma.cpp 890ba99
Diff: https://git.reviewboard.kde.org/r/118898/diff/
Testing
---
Set a gamma value in the KGamma KCM, logout/login (or reboot), Gamma value
gets set correctly.
If there's no kgammarc file (or it contains no actual gamma settings), the
Gamma value is not changed. It stays at what is configured for X (or its
default).
Thanks,
Wolfgang Bauer
Re: Review Request 118898: KGamma: Apply user setting at login/startup
On June 23, 2014, 5:13 p.m., Martin Gräßlin wrote: kcmkgamma/init_kgamma.cpp, line 39 https://git.reviewboard.kde.org/r/118898/diff/1/?file=283881#file283881line39 why delete config? I would just use a KSharedConfig::openConfig Wolfgang Bauer wrote: I just copy/pasted the original code. That is reverted as well now. Should I change this anyway? But that's not really related to this bugfix, I'd say... But that's not really related to this bugfix, I'd say... no, it isn't. It just highlights how bad reviewboard is for copied content - Martin --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/118898/#review60809 --- On June 23, 2014, 7:01 p.m., Wolfgang Bauer wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/118898/ --- (Updated June 23, 2014, 7:01 p.m.) Review request for kde-workspace, Plasma and Marcel Wiesweg. Bugs: 218668 http://bugs.kde.org/show_bug.cgi?id=218668 Repository: kgamma Description --- KGamma's saved user settings are not applied on startup/login. The user has to enter the KCM to apply them. This makes it rather useless, as not even saving the settings system-wide really works any more. (this requires an xorg.conf which normally doesn't exist nowadays) This patch uses kcminit to apply these settings again on login. Apparently this has been forgotten when moving/porting kgamma to KDE4. PS: As there seems to be no kgamma group and this is desktop-related, I decided to add the kde-workspace and plasma groups for review. I hope that's ok... ;) Diffs - kcmkgamma/kgamma.cpp 890ba99 kcmkgamma/kgamma.desktop 3d87513 Diff: https://git.reviewboard.kde.org/r/118898/diff/ Testing --- Set a gamma value in the KGamma KCM, logout/login (or reboot), Gamma value gets set correctly. If there's no kgammarc file (or it contains no actual gamma settings), the Gamma value is not changed. It stays at what is configured for X (or its default). Thanks, Wolfgang Bauer
Re: Bug 92237: patch submitted, but... is anyone watching?
On Sunday 22 June 2014 13:04:44 Simon Bachmann wrote: May I draw your attention to bug 92237? (https://bugs.kde.org/show_bug.cgi?id=92237) No, this is not a please-get-this-issue-fixed-ASAP kind of message. It's just that I submitted a patch to that bug a few weeks ago, and I'm afraid no one noticed (the bug's CC list is empty...) In comment #5 you are asked to use review board. Bugzilla is very bad in tracking or reviewing patches, that's why we have a dedicated application for it. Cheers Martin Gräßlin signature.asc Description: This is a digitally signed message part.
Re: Re: code guideline
On Wednesday 09 July 2014 10:23:59 Kevin Ottens wrote: On Wednesday 09 July 2014 10:15:03 David Faure wrote: On Saturday 28 June 2014 08:51:42 Rodrigo Bonifacio wrote: Dear all, is there any code guideline that recommends developers to avoid the use of exception handling mechanisms within the core libraries of KDE? I don't think it's written down anywhere, but yes, please do avoid the use of exception handling. I hate debugging uncaught exceptions, no way to get a backtrace of where the exception was thrown. Isn't catch throw in gdb just for that? I regularly use it when I've to deal with exceptions. yeah that works, but only if one manually runs gdb. With DrKonqi we just get a rethrown exception and that sucks badly. Cheers Martin signature.asc Description: This is a digitally signed message part.
Re: Review Request 119240: Preventing a crash in the KWindowInfo::Private destructor on OSX
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119240/#review62192 --- Which branch should that go in? I do not know what is currently a release branch. It should probably also be forward ported to frameworks even if the Mac part is currently not compiled (hint: if someone adjusts the code it could be included again ;-) ). kdeui/windowmanagement/kwindowinfo_mac.cpp https://git.reviewboard.kde.org/r/119240/#comment43224 what's the RJVB 20140706 standing for? - Martin Gräßlin On July 12, 2014, 4:22 p.m., Marko Käning wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119240/ --- (Updated July 12, 2014, 4:22 p.m.) Review request for kdelibs, Martin Gräßlin, Ian Wadham, and Thomas Lübking. Bugs: 337154 http://bugs.kde.org/show_bug.cgi?id=337154 Repository: kdelibs Description --- Preventing a crash in the KWindowInfo::Private destructor on OSX (thanks to René J.V. Bertin) Diffs - kdeui/windowmanagement/kwindowinfo_mac.cpp ca502a3c643af36f4a52bb6dcbc7b54fbe3f42a9 Diff: https://git.reviewboard.kde.org/r/119240/diff/ Testing --- For more details see associated bug Thanks, Marko Käning
Re: Review Request 118180: slideshow BUG patch fix
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/118180/#review62779 --- libs/plasmagenericshell/backgrounddialog.cpp https://git.reviewboard.kde.org/r/118180/#comment43523 here the coding style looks wrong - Martin Gräßlin On June 5, 2014, 12:32 p.m., TOM Harrison wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/118180/ --- (Updated June 5, 2014, 12:32 p.m.) Review request for kde-workspace and Plasma. Bugs: 327580 http://bugs.kde.org/show_bug.cgi?id=327580 Repository: kde-workspace Description --- bug patch for slideshow dir list missing Diffs - libs/plasmagenericshell/backgrounddialog.cpp 645de3f Diff: https://git.reviewboard.kde.org/r/118180/diff/ Testing --- Thanks, TOM Harrison
Re: Scripting/Extensions BoF at Akademy?
On Saturday 09 August 2014 17:34:04 Kevin Krammer wrote: Greetings all, I'd like to ask if there is any interest of having a BoF around the topic of script language based extensions for KDE applications. Some applications already offer that, e.g. Kate, and Plasma is even designed around that idea. But currently there seems to be quite some infrastructure implemented multiple times, e.g. a require or include mechanism for QtScript. My goal for the BoF would be to see which functional blocks we have spread across KDE software, if we could identify bits that can be upstreamed and bits that would be nice in a KScriptAddon framework. Count me in to share my experience from KWin. I think we also have there quite some fancy features not available in other applications. Cheers Martin signature.asc Description: This is a digitally signed message part.
Re: Re: Using Gerrit for code review in KDE
On Saturday 13 September 2014 16:51:15 Albert Astals Cid wrote: El Divendres, 12 de setembre de 2014, a les 22:52:40, Marco Martin va escriure: On Tuesday, September 9, 2014, Jan Kundrát j...@flaska.net wrote: If you would like all plasma to go, just give me a list of repos and I can make it happen. No, definitely not yet In my opinion, the purpose of this test is not to verify that Gerrit works or that the ACLs are set up properly -- both were done already. As part of the experiment i would also like to try to have stricter acls for +2 and submit, like starting from mantainers then slowly adding people (that's also how i understood it would have worked during the bof) I'd read that as being against the KDE Manifesto. my understanding was that it's still possible to bypass the code review, so I cannot see that it's against the KDE Manifesto as it's only a kind of social contract. Or am I missing something. Personally I like the idea of having the +2 limited to the devs familiar with the code. Cheers Martin signature.asc Description: This is a digitally signed message part.
