from:"Martin Gräßlin"

D5712: Validate surface is valid when sending TextInput leave event

2017-05-05 Thread Martin Gräßlin

graesslin accepted this revision.
graesslin added a comment.
This revision is now accepted and ready to land.


  Thanks for adding the test!

REPOSITORY
  R127 KWayland

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D5712

To: davidedmundson, #plasma, graesslin
Cc: apol, graesslin, plasma-devel, #frameworks, spstarr, progwolff, Zren, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, lukas

D5712: Validate surface is valid when sending TextInput leave event

2017-05-04 Thread Martin Gräßlin

graesslin requested changes to this revision.
graesslin added a comment.
This revision now requires changes to proceed.


  Please add a unit test case.

REPOSITORY
  R127 KWayland

REVISION DETAIL
  https://phabricator.kde.org/D5712

To: davidedmundson, #plasma, graesslin
Cc: graesslin, plasma-devel, #frameworks, spstarr, progwolff, Zren, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, lukas

Re: KWallet::openWallet and 0-WinId ?

2017-05-02 Thread Martin Gräßlin


Am 2017-05-01 17:33, schrieb Martin Koller:

Hi,

the Wallet::openWallet() documentation says for the WinId argument:
"The window id to associate any dialogs with. You can pass 0 if you
don't have a window the password dialog should associate with."

However passing 0 gives a runtime warning
qDebug() << "Pass a valid window to 
KWallet::Wallet::openWallet().";


So what is correct ?

openWallet() is used by a lot PIM resources and all these show the
warning when akonaditray is not registered
on the DBUS (which means winId is passed as 0)


Simply put: no sane window manager will pass focus to KWallet if it 
doesn't have a window which it can be associated with.


This is the reason why KWallet opens behind (!) other windows and 
unfocused in Plasma when Akonadi tries to interact with it.


So at least in reality allowing 0 is not correct.

Cheers
Martin

D5556: build: Remove KService dependency

2017-04-24 Thread Martin Gräßlin

graesslin accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R268 KGlobalAccel

REVISION DETAIL
  https://phabricator.kde.org/D5556

To: palokisa, graesslin, cfeck, apol
Cc: #frameworks

D5521: Allow the KGlobalAccel be a "Tier 2" framework, if needed

2017-04-21 Thread Martin Gräßlin

graesslin added a comment.


  In https://phabricator.kde.org/D5521#103818, @palokisa wrote:
  
  > In https://phabricator.kde.org/D5521#103810, @graesslin wrote:
  >
  > > The other dependencies: well KCoreAddons is needed for KCrash only IIRC. 
That is setting the KAboutData.
  >
  >
  > I've had a look... and the plugins are also handled by `KPluginLoader`, 
`KPluginMetaData` & co.
  
  
  ah right, so it probably needs to have KCoreAddons.

REPOSITORY
  R268 KGlobalAccel

REVISION DETAIL
  https://phabricator.kde.org/D5521

To: palokisa, graesslin, mck182
Cc: cfeck, apol, #frameworks

D5521: Allow the KGlobalAccel be a "Tier 2" framework, if needed

2017-04-21 Thread Martin Gräßlin

graesslin added a comment.


  oh and yes for removing service dependency please open a separate review. The 
other dependencies: well KCoreAddons is needed for KCrash only IIRC. That is 
setting the KAboutData. So a possibility could be to move the binary out and 
replace it by a binary which does not use KCrash. And we could add the good old 
kglobalacceld5 with KCrash into Plasma. That could solve all the problems we 
have.

REPOSITORY
  R268 KGlobalAccel

REVISION DETAIL
  https://phabricator.kde.org/D5521

To: palokisa, graesslin, mck182
Cc: cfeck, apol, #frameworks

D5521: Allow the KGlobalAccel be a "Tier 2" framework, if needed

2017-04-21 Thread Martin Gräßlin

graesslin added a comment.

  In https://phabricator.kde.org/D5521#103778, @palokisa wrote:

  > In https://phabricator.kde.org/D5521#103775, @cfeck wrote:
  >
  > > Btw, libKF5GlobalAccel is actually Tier1, so applications needing global 
shortcuts will not have any (additional) KF5 dependencies, only the runtime has.
  >
  >
  > But without the runtime the application using libKF5GlobalAccel will not 
get any shortcut signals delivered, not?

  yes and no. Someone needs to provide the dbus interface. Currently we have 
two ways to provide the DBus interface: kglobalacceld5 and kwin_wayland with 
the latter reusing almost everything of the former. So from a technical point 
of view the runtime is not needed, from a practical point of view it is needed.

  That's also a reason why we moved the runtime into kglobalaccel as it's kind 
of pointless to have the framework without the runtime.

REPOSITORY
  R268 KGlobalAccel

REVISION DETAIL
  https://phabricator.kde.org/D5521

To: palokisa, graesslin, mck182
Cc: cfeck, apol, #frameworks

D5521: Allow the KGlobalAccel be a "Tier 2" framework, if needed

2017-04-20 Thread Martin Gräßlin

graesslin added a comment.

  In https://phabricator.kde.org/D5521#103661, @palokisa wrote:

  > > Distributions will ship only one variant of kglobalaccel and that will 
most likely be the one which is wanted by KDE
  >
  > Why is that? Why can't we have e.g. one `foo-kde` and other `foo-lxqt` 
package, both providing virtual `foo` (these will be in conflict as they will 
provide the same file(s)). Then KDE can depend strictly on `foo-kde`.

  Talk to distros. My experience is that this will be an absolute no-go for 
most distros. You might find one or two, but I doubt any of the major 
distributions will do that. The obvious reason is that it would be impossible 
to install frameworks and lxqt at the same time. Note that this is about 
frameworks and not even Plasma. Plasma has no play in kglobalaccel.

REPOSITORY
  R268 KGlobalAccel

REVISION DETAIL
  https://phabricator.kde.org/D5521

To: palokisa, graesslin, mck182
Cc: apol, #frameworks

D5521: Allow the KGlobalAccel be a "Tier 2" framework, if needed

2017-04-20 Thread Martin Gräßlin

graesslin added a comment.


  I don't really understand what this change is supposed to fix. Distributions 
will ship only one variant of kglobalaccel and that will most likely be the one 
which is wanted by KDE. If kglobalaccel is shipped without KCrash support I 
would consider this as a serious problem and report that to the distributions. 
Also given my experience about breakage in weird situations I'm against such 
build flexibility. KCrash is an important component for kglobalaccel and I'm 
not interested in having to spend time on bug reports because a distro 
mis-configured kglobalaccel.

REPOSITORY
  R268 KGlobalAccel

REVISION DETAIL
  https://phabricator.kde.org/D5521

To: palokisa, graesslin, mck182
Cc: apol, #frameworks

D5503: No "KDE Daemon" in password dialogs

2017-04-19 Thread Martin Gräßlin

graesslin added a comment.


  not that "KDE Daemon" is a good name, but what about i18n it?

REPOSITORY
  R297 KDED

REVISION DETAIL
  https://phabricator.kde.org/D5503

To: lukas, #frameworks
Cc: graesslin, apol, mck182

Re: KAuth buildability: new CI architecture

2017-04-17 Thread Martin Gräßlin


Am 2017-04-17 06:49, schrieb Ben Cooksley:
On Mon, Apr 17, 2017 at 12:48 AM, Martin Gräßlin <mgraess...@kde.org> 
wrote:

Am 2017-04-16 13:52, schrieb Ben Cooksley:


On Sun, Apr 16, 2017 at 11:09 PM, Harald Sitter <sit...@kde.org> 
wrote:


Not particularly related to the issue at hand (which is probably
polkitqt having meh cmake files), but relocating stuff in general is
sper unreliable and I would absolutely advise against it as it 
can
easily screw up test results and builds alike, often in unobvious 
ways

(all it takes is a bit of libexec use). Instead, as a general
principle, I would suggest that you get stuff mounted in suitably
stable/consistent/generic paths inside the build containers.
Ultimately what things look like natively on the host file system
shouldn't factor into what they look like in the build environment.



While I have thought of using a consistent path, this would simply
workaround the fact that our binaries are frail and have hardcoded
paths baked into them.



note that this is partially intended for security reasons. E.g.
kscreenlocker starts kcheckpass through a hardcoded path for security
reasons (we want to be sure it's the kcheckpass we created and not a
fakekcheckpass injected by a malicious tool). So overall especially in
plasma lots of Wayland stuff is hardcoding paths for this reason and
partially they are used in testing.

In the case of kscreenlocker this can become a problem when KWin tests 
are
run. We have tests verify that locking the screen works on Wayland. 
For that
kscreenlocker_greet gets started and that has a hardcoded path as 
well. So

if the paths is relocated we test an unsupported setup.


Would it be possible to use relative-to-calling-binary paths?


Simply put: no. That would require quite some engineering effort 
especially considering that distros do have the libexec paths different 
with some adding the arch into it. This makes it almost impossible to 
get it right.


So in the case of kscreenlocker I have absolute zero interest in 
re-engineering it to support relative paths. The current code works and 
we can just assume that it is there. Anything else requires error 
handling and if something goes wrong users have a system which cannot be 
unlocked. The risk is not worth the effort.


Cheers
Martin

Re: KAuth buildability: new CI architecture

2017-04-16 Thread Martin Gräßlin


Am 2017-04-16 13:52, schrieb Ben Cooksley:

On Sun, Apr 16, 2017 at 11:09 PM, Harald Sitter  wrote:

Not particularly related to the issue at hand (which is probably
polkitqt having meh cmake files), but relocating stuff in general is
sper unreliable and I would absolutely advise against it as it can
easily screw up test results and builds alike, often in unobvious ways
(all it takes is a bit of libexec use). Instead, as a general
principle, I would suggest that you get stuff mounted in suitably
stable/consistent/generic paths inside the build containers.
Ultimately what things look like natively on the host file system
shouldn't factor into what they look like in the build environment.


While I have thought of using a consistent path, this would simply
workaround the fact that our binaries are frail and have hardcoded
paths baked into them.


note that this is partially intended for security reasons. E.g. 
kscreenlocker starts kcheckpass through a hardcoded path for security 
reasons (we want to be sure it's the kcheckpass we created and not a 
fakekcheckpass injected by a malicious tool). So overall especially in 
plasma lots of Wayland stuff is hardcoding paths for this reason and 
partially they are used in testing.


In the case of kscreenlocker this can become a problem when KWin tests 
are run. We have tests verify that locking the screen works on Wayland. 
For that kscreenlocker_greet gets started and that has a hardcoded path 
as well. So if the paths is relocated we test an unsupported setup.


Cheers
Martin

D5405: Create desktop file name based on organization domain unless set explicitely

2017-04-12 Thread Martin Gräßlin

graesslin added a comment.


  In https://phabricator.kde.org/D5405#101628, @stikonas wrote:
  
  > Ok, so let us not change kcoreaddons now although it's still something that 
could have better default behaviour in KF6.
  
  
  My comment was not meant as a stop-this-effort comment. If we can improve 
here to do more sane things I'm all for it.

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D5405

To: stikonas, mpyne, kossebau, aacid, ltoscano
Cc: nalvarez, graesslin, mak, plasma-devel, kde-frameworks-devel, #frameworks, 
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol

D5405: Create desktop file name based on organization domain unless set explicitely

2017-04-12 Thread Martin Gräßlin

graesslin added a comment.


  In https://phabricator.kde.org/D5405#101626, @ltoscano wrote:
  
  > In https://phabricator.kde.org/D5405#101624, @graesslin wrote:
  >
  > > In https://phabricator.kde.org/D5405#101621, @ltoscano wrote:
  > >
  > > > In https://phabricator.kde.org/D5405#101615, @graesslin wrote:
  > > >
  > > > > The desktop file name should follow the way how the dbus name is 
created. If the applications are broken, then they are broken. Given that I 
mentioned this several times at KDE conferences, blogged about it, sent mails 
to KDE devel lists I assume the application maintainers don't care whether 
their applications work on Wayland. Which is totally fine. Then let them stay 
broken.
  > > >
  > > >
  > > > When you talk about "desktop file name", do you mean the method 
desktopFileName() in KAboutData, or the real file name of the desktop file?
  > >
  > >
  > > Here I meant the method in Kaboutdata.
  > >
  > > > Also: I did not maintain an application back then and I missed those 
notifications. Could you please link at least one reference to the explanation 
(or which Akademy)? I will get the others from that.
  > >
  > > 
https://blog.martin-graesslin.com/blog/2015/07/porting-qt-applications-to-wayland/
  >
  >
  > Thanks. I guess it's the section "Setting window icon", but:
  >
  > - there are no references to D-Bus, which you referred to in the previous 
comment (not also in the rest of the page in this context);
  
  
  Yes. I didn't want to imply that it must be the same as dbus. I think our 
code should apply the same logic to generate dbus name and desktop file name.
  
  > - the "domain name" link refers to organizationDomain. The problem here is 
that the icon was not visible in applications were  organizationDomain was 
properly set, because the homepage was used when setting the desktop file name.
  
  It might be that this used to work back then or that I was not aware of the 
problem.
  
  > So setDesktopFileName must be used explicitly, and maybe it was not 
available when you wrote the blog post, could you please add a note there to 
reference that method?
  
  Yes I added setDesktopFileName after that blog post. I normally do not modify 
old blog posts as they should not be our documentation.

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D5405

To: stikonas, mpyne, kossebau, aacid, ltoscano
Cc: nalvarez, graesslin, mak, plasma-devel, kde-frameworks-devel, #frameworks, 
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol

D5405: Create desktop file name based on organization domain unless set explicitely

2017-04-12 Thread Martin Gräßlin

graesslin added a comment.


  In https://phabricator.kde.org/D5405#101621, @ltoscano wrote:
  
  > In https://phabricator.kde.org/D5405#101615, @graesslin wrote:
  >
  > > The desktop file name should follow the way how the dbus name is created. 
If the applications are broken, then they are broken. Given that I mentioned 
this several times at KDE conferences, blogged about it, sent mails to KDE 
devel lists I assume the application maintainers don't care whether their 
applications work on Wayland. Which is totally fine. Then let them stay broken.
  >
  >
  > When you talk about "desktop file name", do you mean the method 
desktopFileName() in KAboutData, or the real file name of the desktop file?
  
  
  Here I meant the method in Kaboutdata.
  
  > Also: I did not maintain an application back then and I missed those 
notifications. Could you please link at least one reference to the explanation 
(or which Akademy)? I will get the others from that.
  
  
https://blog.martin-graesslin.com/blog/2015/07/porting-qt-applications-to-wayland/

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D5405

To: stikonas, mpyne, kossebau, aacid, ltoscano
Cc: nalvarez, graesslin, mak, plasma-devel, kde-frameworks-devel, #frameworks, 
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol

D5405: Create desktop file name based on organization domain unless set explicitely

2017-04-12 Thread Martin Gräßlin

graesslin added a comment.


  The desktop file name should follow the way how the dbus name is created. If 
the applications are broken, then they are broken. Given that I mentioned this 
several times at KDE conferences, blogged about it, sent mails to KDE devel 
lists I assume the application maintainers don't care whether their 
applications work on Wayland. Which is totally fine. Then let them stay broken.

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D5405

To: stikonas, mpyne, kossebau, aacid, ltoscano
Cc: graesslin, mak, plasma-devel, kde-frameworks-devel, #frameworks, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol

D5174: Add support for wl_shell_surface::set_popup and popup_done

2017-03-28 Thread Martin Gräßlin

This revision was automatically updated to reflect the committed changes.
Closed by commit R127:6c89a61d2d17: Add support for wl_shell_surface::set_popup 
and popup_done (authored by graesslin).

REPOSITORY
  R127 KWayland

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D5174?vs=12795=12924

REVISION DETAIL
  https://phabricator.kde.org/D5174

AFFECTED FILES
  autotests/client/test_wayland_shell.cpp
  src/client/shell.cpp
  src/client/shell.h
  src/server/shell_interface.cpp
  src/server/shell_interface.h

To: graesslin, #plasma_on_wayland, #frameworks, #kwin, mart
Cc: plasma-devel, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
eliasp, sebas, apol

D5174: Add support for wl_shell_surface::set_popup and popup_done

2017-03-25 Thread Martin Gräßlin

graesslin added a dependent revision: D5177: Initial support for popup window 
handling.

REPOSITORY
  R127 KWayland

REVISION DETAIL
  https://phabricator.kde.org/D5174

To: graesslin, #plasma_on_wayland, #frameworks, #kwin
Cc: plasma-devel, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
eliasp, sebas, apol

D5174: Add support for wl_shell_surface::set_popup and popup_done

2017-03-25 Thread Martin Gräßlin

graesslin created this revision.
Restricted Application added a subscriber: plasma-devel.
Restricted Application added projects: Plasma on Wayland, Frameworks.

REVISION SUMMARY
  This extends the client side API to support creating popup ShellSurface
  windows and the server side API to send out the popup_done request.
  
  This is needed to properly support popup windows (e.g. context menus)
  in KWin.

REPOSITORY
  R127 KWayland

BRANCH
  popup-done

REVISION DETAIL
  https://phabricator.kde.org/D5174

AFFECTED FILES
  autotests/client/test_wayland_shell.cpp
  src/client/shell.cpp
  src/client/shell.h
  src/server/shell_interface.cpp
  src/server/shell_interface.h

To: graesslin, #plasma_on_wayland, #frameworks, #kwin
Cc: plasma-devel, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
eliasp, sebas, apol

D5134: Don't exit if $DISPLAY is not set

2017-03-22 Thread Martin Gräßlin

graesslin requested changes to this revision.
graesslin added a comment.
This revision now requires changes to proceed.


  As I stumbled over that years ago: sorry I don't think that this is the 
proper solution. This is basically https://phabricator.kde.org/T4431 which is 
not yet implemented. What this change suggest is a rather hacky solution.
  
  Kinit if built with X11 is still requiring the DISPLAY variable all over the 
place. The only solution is to run an X server, even if it is not the X 
platform As we do on Plasma/Wayland

REPOSITORY
  R303 KInit

REVISION DETAIL
  https://phabricator.kde.org/D5134

To: aacid, graesslin
Cc: graesslin, rikmills, lukas, #frameworks

Re: KGuiAddons and QGestures like tap/click-and-hold to open context menu?

2017-03-13 Thread Martin Gräßlin



Am 13. März 2017 16:15:34 MEZ schrieb Kai Uwe Broulik :
>‎
>> Macs however always have two-fingers secondary click; their magic
>mouse have both (it's mouse-touchpad hybrid).
>
>If all else fails you could still Control click on Mac iirc.
>
>This feature would be useful for Plasma on touchscreens though, faking
>a right click on  *touch* long-press. Maybe that's an approach that's
>more pleasing to Martin G than doing that inside KWin which he said is
>the wrong place to do that.

It is not the wrong place, it is technically impossible to do it in KWin.
Generating context menu events must be in the applications. Only those can know 
what makes sense and know whether the touch event is handled or not.

In general I oppose the idea. I think we need to rethink and not emulating a 
decades old pattern, just because we don't have a better idea.
Especially in Plasma I would not like to see this. We can do better.

Cheers
Martin

[Differential] [Changed Subscribers] D4847: KAuth integration in document saving

2017-03-01 Thread Martin Gräßlin

graesslin added subscribers: davidedmundson, graesslin.
graesslin added a comment.


  I really like what I see here!
  
  Just a thought: what happens if the file is not owned by root, but e.g. by 
www-data? If I understand the code correctly it might change to be owned by 
root due to the usage of QSaveFile?
  
  I would like to see some KAuth experts (e.g. @davidedmundson ) to have a good 
look at the code to verify that it doesn't introduce vulnerabilities. As far as 
I studied the code I didn't see anything bad.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D4847

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: martinkostolny, #ktexteditor
Cc: graesslin, davidedmundson, palant, kwrite-devel, #frameworks, head7, 
cullmann, kfunk, sars, dhaumann

Re: kwin requires qt5.7.0

2017-02-26 Thread Martin Gräßlin


Am 2017-02-26 18:00, schrieb Allen Winter:

I thought Qt5.6 was the minimum required?
Just asking.  I don't care that much but I need to install Qt5.7 ,
which of course I can do.


As Luigi already wrote: wrong list for this topic. KWin is part of 
Plasma and follows Plasma's minimum Qt requirement, which is for Plasma 
5.10 (!) Qt 5.7. The last stable release of KWin (5.9) only had Qt 5.5 
as minimum.


Cheers
Martin

Re: Review Request 126724: Expose callingUser in HelperSupport if available

2017-02-26 Thread Martin Gräßlin



> On Feb. 25, 2017, 11:53 p.m., Albert Astals Cid wrote:
> > Martin any reason this was not commmited? Should I?

If I remember correctly it depends on a newer polkit-qt functionality and 
polkit-qt hasn't seen a release yet. Unfortunately I cannot really check as the 
diff now only shows whitespace changes (yay for reviewboard to break the diffs)


- Martin


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126724/#review102607
---


On Jan. 12, 2016, 10:36 a.m., Martin Gräßlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126724/
> ---
> 
> (Updated Jan. 12, 2016, 10:36 a.m.)
> 
> 
> Review request for KDE Frameworks, Dario Freddi and David Edmundson.
> 
> 
> Repository: kauth
> 
> 
> Description
> ---
> 
> The Polkit backend is able to retrieve the calling user. As this is
> a useful information for a helper the information can be exposed in
> the AuthBackend and be retrieved through the HelperSupport.
> 
> 
> Diffs
> -
> 
>   src/AuthBackend.h c67a706dda107c9815e3c3f74c628fed6d2e4dcc 
>   src/AuthBackend.cpp ff91dd580919af9046b3f3d26f340885d54d370e 
>   src/CMakeLists.txt 1b6930d1db89f6ecc1223772b6632c57762f829f 
>   src/backends/polkit-1/Polkit1Backend.cpp 
> 78ee5bb6d97d9d83beec21e197a947dfc994b2a9 
>   src/kauthhelpersupport.h 2828ec21aa00b7fb35dcf19db7f3869158e85b1d 
>   src/kauthhelpersupport.cpp c2a88d7cc574eb6ab092f0981b828fd4c68ba025 
> 
> Diff: https://git.reviewboard.kde.org/r/126724/diff/
> 
> 
> Testing
> ---
> 
> a helper with:
> ActionReply KScreenLockerAuthHelper::save(const QVariantMap )
> {
> auto user = KAuth::HelperSupport::callingUser();
> 
> QFile file(QStringLiteral("/tmp/authtest"));
> file.open(QIODevice::WriteOnly);
> file.write(user->homeDir().toUtf8());
> file.write("\n");
> file.write(user->loginName().toUtf8());
> file.write("\n");
> file.write(QByteArray::number(user->userId().nativeId()));
> file.write("\n");
> 
> return ActionReply::SuccessReply();
> }
> 
> created the file /tmp/authtest with the following content:
> 
> /home/martin
> martin
> 1000
> 
> Which matches my user.
> 
> 
> Thanks,
> 
> Martin Gräßlin
> 
>

[Differential] [Commented On] D4799: Delay notifications until desktop session has loaded

2017-02-25 Thread Martin Gräßlin

graesslin added a comment.


  I would say it is enough to just make plasma grab the interface early enough. 
It would result in the popups not show.
  
  We don't need to show those notifications. It is a bug in ksplash that the 
popups are visible at all.
  
  Another alternative could be to bind the checks suggested here to the env 
variable KDE_FULL_SESSION to make it at least a plasma only penalty

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D4799

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: vpilo
Cc: graesslin, mck182, #frameworks

Re: Review Request 129967: Make KGlobalaccel X11 track XInput events to still work when an application grabs the keyboard

2017-02-24 Thread Martin Gräßlin


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129967/#review102585
---



As an idea: if I understand correctly the main motivation is to be able to 
trigger the volume control keys while a context menu is open. You are not that 
much interested in e.g. triggering Alt+Tab while context menu is shown.

So a possibility might be to add this to the client side. Assuming the Qt case: 
if keyboard is grabbed, context menu is open and the key event is not handled 
and one of the special keys which the application does not care about, it could 
close the context menu and XTest the event again. Then kglobalaccel would be 
able to handle it.

- Martin Gräßlin


On Feb. 24, 2017, 5:26 p.m., David Edmundson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129967/
> ---
> 
> (Updated Feb. 24, 2017, 5:26 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kglobalaccel
> 
> 
> Description
> ---
> 
> This means shortcuts still work when keyboard is grabbed by another
> application. This allows us to still take a screenshot/change volume
> with a context menu open.
> 
> The existing code doesn't seem to actually handle dynamic keyboard
> layout switching, this code matches that behaviour.
> 
> Interestingly alt+tab behavior is unchanged so it doesn't /severely/ break
> legitimate keyboard grabs like virtualbox.
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt 0de00d4a6b0700a0c36fe291a8240bea43e62cc2 
>   cmake/modules/FindXKB.cmake PRE-CREATION 
>   src/runtime/plugins/xcb/CMakeLists.txt 
> 45bf4dc9f9ec9146a0db5f93b4259ea6c4306d17 
>   src/runtime/plugins/xcb/kglobalaccel_x11.h 
> 88b7f61e9cfa5e30df7f04f492a864f1ce8eb37a 
>   src/runtime/plugins/xcb/kglobalaccel_x11.cpp 
> 9b37c7b600d6f25a0ea43643d8dee2593c3b7417 
> 
> Diff: https://git.reviewboard.kde.org/r/129967/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

Re: Review Request 129967: Make KGlobalaccel X11 track XInput events to still work when an application grabs the keyboard

2017-02-24 Thread Martin Gräßlin

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129967/#review102584
---

This introduces a security issue as it would be possible to trigger global 
shortcuts while the screen is locked. Given that I am reluctant to accept it. 
This will also mean lots of bug reports raised due to it. Also it can result in 
severe issues in code which currently can savely assume that no global shortcut 
gets trigger. Large parts of KWin rely on the knowledge that no global shortcut 
gets triggered. E.g. Present Windows and Alt+Tab are mutual exclusive due to 
the keyboard grab. This change would result in undefined behavior in such a 
situation. It is not known how the code would react as that is outside the 
specification. We don't know how many more applications rely on keyboard 
grabbing not triggering global shortcuts.

For what is worth: on Wayland it is possible to trigger global shortcuts while 
context menus are open. I think now it is better to spend the time on making 
Wayland a first class citizen than to try working around X11 limitations.

- Martin Gräßlin

On Feb. 24, 2017, 5:26 p.m., David Edmundson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129967/
> ---
> 
> (Updated Feb. 24, 2017, 5:26 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kglobalaccel
> 
> 
> Description
> ---
> 
> This means shortcuts still work when keyboard is grabbed by another
> application. This allows us to still take a screenshot/change volume
> with a context menu open.
> 
> The existing code doesn't seem to actually handle dynamic keyboard
> layout switching, this code matches that behaviour.
> 
> Interestingly alt+tab behavior is unchanged so it doesn't /severely/ break
> legitimate keyboard grabs like virtualbox.
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt 0de00d4a6b0700a0c36fe291a8240bea43e62cc2 
>   cmake/modules/FindXKB.cmake PRE-CREATION 
>   src/runtime/plugins/xcb/CMakeLists.txt 
> 45bf4dc9f9ec9146a0db5f93b4259ea6c4306d17 
>   src/runtime/plugins/xcb/kglobalaccel_x11.h 
> 88b7f61e9cfa5e30df7f04f492a864f1ce8eb37a 
>   src/runtime/plugins/xcb/kglobalaccel_x11.cpp 
> 9b37c7b600d6f25a0ea43643d8dee2593c3b7417 
> 
> Diff: https://git.reviewboard.kde.org/r/129967/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

Re: Global shortcuts should not send keyrelease event to app with focus (Re: KStandardAction::rename ?)

2017-02-19 Thread Martin Gräßlin



Am 19. Februar 2017 18:00:07 MEZ schrieb David Faure <fa...@kde.org>:
>On dimanche 19 février 2017 12:51:39 CET Martin Gräßlin wrote:
>> Am 19. Februar 2017 11:21:18 MEZ schrieb David Faure <fa...@kde.org>:
>> >Result: the test app shows the keypress events for Ctrl and Alt
>> >(obviously),
>> >it does not show the keypress for K (good, this matches my
>> >expectation),
>> >BUT when I release K, the app receives a key *RELEASE* event for
>Key_K.
>> >
>> >Martin, is this a bug and is it fixable?
>> 
>> That is not a bug and is the expected and correct behavior. 
>
>Can you elaborate why this is expected? If I press a workspace-wide
>global 
>shortcut like Alt+F2 or Ctrl+Alt+K, then I intend for that to go to the
>
>workspace, not to the window with focus. The fact that this window does
>not 
>receive a keypress but receives a keyrelease for that key seems very 
>inconsistent and unexpected to me.

That is how X works *shrug*. It is quite normal that applications can get key 
release events for keys they did not get a press for.

Press a key then switch focus using the mouse and release the key. Same thing.

Also on Wayland quite normal. Except that the protocol informs which keys are 
currently pressed. But iirc Qt (rightly) ignores that information.

The application is at fault if it acts on a release without a press.

>
>> Also the window gets a focus out and focus on event.
>
>OK but that doesn't prevent processing the key release event.

[Differential] [Accepted] D4637: KColorScheme: read application's KDE_COLOR_SCHEME_PATH property

2017-02-19 Thread Martin Gräßlin

graesslin accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R265 KConfigWidgets

REVISION DETAIL
  https://phabricator.kde.org/D4637

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: flherne, #frameworks, graesslin
Cc: graesslin

[Differential] [Commented On] D4637: KColorScheme: read application's KDE_COLOR_SCHEME_PATH property

2017-02-17 Thread Martin Gräßlin

graesslin added inline comments.

INLINE COMMENTS

> kcolorscheme.h:316-318
> + * from the given KConfig. If null, the application's 
> KDE_COLOR_SCHEME_PATH
> + * property will be used to load a KConfig. If this is also unset, the
> + * system colors will be used.

I wouldn't mention the property name - that can be considered an implementation 
detail. Maybe instead something like "If a color scheme got installed with a 
KColorSchemeManager that one is used".

REPOSITORY
  R265 KConfigWidgets

REVISION DETAIL
  https://phabricator.kde.org/D4637

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: flherne, #frameworks
Cc: graesslin

[Differential] [Commented On] D4637: KColorScheme: read application's KDE_COLOR_SCHEME_PATH property

2017-02-17 Thread Martin Gräßlin

graesslin added a comment.


  > That's not new, the property's been set here since at least the kdelibs 
split.
  
  I know, I was the one adding it :-) That's also why I think we could do that 
better than a property: it was intended as a way to communicate with the QStyle 
- the aim of the property is to inform kwin which color scheme the window uses 
and to adjust the decoration.
  
  So if we use this in more places it might be better to not go through a 
property, but through a proper manager instance. But that's totally orthogonal 
to your change, of course.

REPOSITORY
  R265 KConfigWidgets

REVISION DETAIL
  https://phabricator.kde.org/D4637

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: flherne, #frameworks
Cc: graesslin

[Differential] [Commented On] D4637: KColorScheme: read application's KDE_COLOR_SCHEME_PATH property

2017-02-16 Thread Martin Gräßlin

graesslin added a comment.


  This is a sensible idea, though I wonder whether we should put it into 
something more concrete than a QProperty on the qApp.

INLINE COMMENTS

> kcolorscheme.h:33-37
> +KSharedConfigPtr defaultConfig() {
> +// Read from the application's color scheme file (as set by 
> KColorSchemeManager).
> +// If unset, this is equivalent to openConfig() and the system scheme is 
> used.
> +return 
> KSharedConfig::openConfig(qApp->property("KDE_COLOR_SCHEME_PATH").toString());
> +}

in case the kcolorscheme.h gets installed, I suggest to not inline the method.

REPOSITORY
  R265 KConfigWidgets

REVISION DETAIL
  https://phabricator.kde.org/D4637

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: flherne, #frameworks
Cc: graesslin

[Differential] [Commented On] D4587: [ContainmentInterface] Ungrab mouse on context menu close

2017-02-12 Thread Martin Gräßlin

graesslin added a comment.


  > Looks like Qt 5.8 has a grabber bug
  
  Then Qt should fix and not we workaround it.
  
  Also: how do you know whether your change will still work in Qt 5.7?

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D4587

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: anthonyfieroni, #plasma
Cc: graesslin, plasma-devel, davidedmundson, mart, #frameworks, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol

Re: Trusting .desktop files

2017-02-10 Thread Martin Gräßlin


Am 2017-02-10 19:56, schrieb Fabian Vogt:

Hi,

The reddit post "How to easily trick $FILE_MANAGER users to execute
arbitrary code"
(https://www.reddit.com/r/linux/comments/5r6va0) spawned a discussion
about .desktop files.


Thanks for bringing up this important topic! (Although I get more and 
more annoyed how bug reporting moves to reddit :-P)



What I'm proposing instead is to keep a list of trusted Exec= values
and ask the user for confirmation
everytime a .desktop file with an unknown Exec= gets opened. 
Advantages:


- (Minor, does not usually happen) Changing Exec= revokes the 
trustedness.

- Copying .desktop files just works. Currently DnD'd .desktop files
from /usr/share/applications/
  onto the desktop are untrusted by default.
- The prompt shown when opening an untrusted file specifically shows
only the Exec= value.
  So it's also the Exec= value the user trusts and not the .desktop 
file.

- Cannot be faked by archives.

As Exec= can also contain relative paths, the working directory needs
to be accounted for as well.

Thoughts, suggestions?


What I don't like in general is that this is all happening as $user. 
Thus any malicious program running as $user can also just change the 
list of trusted Exec= values.


So my suggestion is: let's use polkit.

The list of trusted .desktop files must be root owned and per user. 
Everytime a user asks for executing a not known (or changed) desktop 
file, it goes through polkit. To detect changes of the desktop file I 
would suggest to store the shasum of the desktop file in addition. This 
would prevent malicious programs to just change the desktop file.


What do you think? Sensible? Too much?

Cheers
Martin

[Differential] [Commented On] D4416: Send desktopfilename as part of notifyByPopup hints

2017-02-04 Thread Martin Gräßlin

graesslin added a comment.


  In https://phabricator.kde.org/D4416#83019, @mck182 wrote:
  
  > In https://phabricator.kde.org/D4416#82951, @hein wrote:
  >
  > > Gnome-only spec? Why aren't they contributing to fd.o?
  >
  >
  > "Galago is dead, we took over, deal with it."
  >
  > https://bugzilla.gnome.org/show_bug.cgi?id=745634#c25
  
  
  why is it always Matthias Classen?

REPOSITORY
  R289 KNotifications

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D4416

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: davidedmundson, #plasma, apol
Cc: graesslin, mck182, hein, plasma-devel, #frameworks, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas

Re: Phabricator: All repositories registered - upcoming workflow changes

2017-02-01 Thread Martin Gräßlin



Am 1. Februar 2017 20:34:52 MEZ schrieb Ben Cooksley :
>On Wed, Feb 1, 2017 at 9:48 PM, Milian Wolff  wrote:
>> On Tuesday, January 31, 2017 7:56:52 PM CET Ben Cooksley wrote:
>>> On Tue, Jan 31, 2017 at 11:36 PM, René J.V. Bertin
>
>> wrote:
>>> > On Sunday January 29 2017 08:32:21 Ben Cooksley wrote:
>>> >
>>> > Hi,
>>>
>>> Hi Rene,
>>>
>>> > >From this point forward, communities should be moving away from
>>> >>
>>> >>Reviewboard to Phabricator for conducting code review. Sysadmin
>will
>>> >>be announcing a timeline for the shutdown of Reviewboard in the
>near
>>> >>future.
>>> >>
>>> > I hope that shutdown doesn't mean complete disconnect; it would
>probably
>>> > be a loss of as-yet unknown importance if all code reviews become
>>> > unavailable.
>>> >
>>> > I'll miss ReviewBoard. Phabrithingy may be more powerful and
>versatile,
>>> > but RB had its advantages too which could be why it's still being
>used
>>> > (quite a lot, as far as I can see) and hasn't been integrated with
>KDE's
>>> > own IDE yet.
>>>
>>> It will be a complete shutdown of Reviewboard - we'll be archiving
>it
>>> in the event for some reason it becomes necessary to access the data
>>> it stores.
>>
>> This is a *very* bad idea!
>>
>> - Quite some commits will lose some extended history from the review
>comments
>> - What about the not-yet-merged changes?
>
>There will sufficient time between now and when the shutdown is
>actually actioned during which it's expected any remaining reviews can
>either be finished off, or moved to Phabricator (As said in my
>original mail, we'll be publishing a timeline for this - which will
>have stages where no new reviews can be opened, etc)
>
>>
>> If at all possible, please find a way to keep this site alive in a
>read-only
>> mode.
>>
>>> In most cases mailing lists should have the history of reviews in
>>> their archives, so those will continue to be accessible through list
>>> archives in the long run.
>>
>> And how do you find the corresponding mail archive thread based on a
>> reviewboard URL? Will there be auto-forwarding in-place?
>
>There won't be any auto-forwarding - we'll be removing the subdomains
>completely.

Could you please reconsider? Every fixed KWin bug report has a link to the 
review request where the patch is discussed.

Losing this would be a huge blow to our source code and bug report history.

Please leave the history of review request around.

Thank you
Martin

Re: Review Request 129842: KAuth: update most of the examples, drop outdated ones

2017-01-28 Thread Martin Gräßlin


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129842/#review102297
---


Ship it!




Great work. KAuth was really lacking documentation. I remember me cursing about 
the documentation being wrong quite often during the Qt 5 port.

- Martin Gräßlin


On Jan. 15, 2017, 6:10 p.m., Elvis Angelaccio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129842/
> ---
> 
> (Updated Jan. 15, 2017, 6:10 p.m.)
> 
> 
> Review request for KDE Frameworks and Aleix Pol Gonzalez.
> 
> 
> Repository: kauth
> 
> 
> Description
> ---
> 
> This moves most of the example snippets (which were outdated) into real cmake 
> targets. We use the Doxygen `@snippet` magic to show them in the apidox. This 
> ensures that the example code is always compiled and stays up-to-date.
> Some of the snippets are just removed, in particular the section about 
> executing the actions asynchronously, which predates the KJob (ExecuteJob) 
> usage.
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt 58d0f2a5d6c2f442097a27e229a1b8cb3ec35f73 
>   examples/CMakeLists.txt PRE-CREATION 
>   examples/client.cpp PRE-CREATION 
>   examples/helper.cpp PRE-CREATION 
>   metainfo.yaml f709143c00d70c834d6ccde103f575be3771d448 
>   src/kauthactionreply.h 6203df034483c102eb82220cca3dac313a094f41 
> 
> Diff: https://git.reviewboard.kde.org/r/129842/diff/
> 
> 
> Testing
> ---
> 
> Examples targets build fine.
> Ran kapidox locally, code snippets shows up in the documentation of the KAuth 
> namespace.
> 
> 
> Thanks,
> 
> Elvis Angelaccio
> 
>

Re: Review Request 126758: Drop support for building with Qt 4

2017-01-20 Thread Martin Gräßlin


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126758/
---

(Updated Jan. 20, 2017, 11:25 p.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks and David Edmundson.


Repository: polkit-qt-1


Description
---

With this change only building with Qt5 is supported.


Diffs
-

  CMakeLists.txt bb91bdedc96b8211eb29a1180c2e451dc60fae18 
  agent/CMakeLists.txt 51cb1d5e7cdb6db36890846ed5d4e2d024cef8d8 
  core/CMakeLists.txt 9da20d7c909632eb949a90f8c594293c292814e0 
  examples/CMakeLists.txt bdfa8cafd273476d7219c860972e5fce721050a7 
  examples/agent/CMakeLists.txt fdaf26edd43723a3301a4eae99da6f9989899d6f 
  gui/CMakeLists.txt 8d1d537ebedcaab4ec9e54824186c3be83434f12 
  test/CMakeLists.txt bc439658a623160522259485c89b39661e61 

Diff: https://git.reviewboard.kde.org/r/126758/diff/


Testing
---

compiles


Thanks,

Martin Gräßlin

[Differential] [Commented On] D4201: Make it possible to lower KCrash to tier 1

2017-01-19 Thread Martin Gräßlin

graesslin added a comment.


  In https://phabricator.kde.org/D4201#78639, @apol wrote:
  
  > In https://phabricator.kde.org/D4201#78631, @graesslin wrote:
  >
  > > I don't understand why a plugin architecture allows to move it to tier1. 
Is functionality split out? if yes, how does that help?
  >
  >
  > Ah, you are right, I forgot to mention it. I thought about this process as 
a 2-step, where the second step would be moving these handlers to 
`frameworksintegration`.
  
  
  Hmm, I don't like that. That turns frameworksintegration into the everything 
and the kitchen sink. Why should one depend on KIO to get a working KCrash?
  
  In the end that results in worse dependencies than before.

REPOSITORY
  R285 KCrash

REVISION DETAIL
  https://phabricator.kde.org/D4201

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: apol, #frameworks, dfaure
Cc: anthonyfieroni, graesslin

[Differential] [Commented On] D4201: Make it possible to lower KCrash to tier 1

2017-01-19 Thread Martin Gräßlin

graesslin added a comment.


  I don't understand why a plugin architecture allows to move it to tier1. Is 
functionality split out? if yes, how does that help?

INLINE COMMENTS

> kstartupinfoplugin.cpp:26
> +{
> +Q_OBJECT\
> +Q_PLUGIN_METADATA(IID "org.kde.KCrashHandlerPlugin")

is the \ intended?

REPOSITORY
  R285 KCrash

REVISION DETAIL
  https://phabricator.kde.org/D4201

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: apol, #frameworks, dfaure
Cc: graesslin

[Differential] [Changed Subscribers] D4175: Introduce a QCommandLineParser for kglobalaccel5

2017-01-17 Thread Martin Gräßlin

graesslin added inline comments.

INLINE COMMENTS

> main.cpp:63
>  
> +{
> +QCommandLineParser parser;

Why did you scope the parser?

REPOSITORY
  R268 KGlobalAccel

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D4175

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: apol, #frameworks, davidedmundson
Cc: graesslin

[Differential] [Accepted] D4141: Allow Tab as being modified by Shift

2017-01-15 Thread Martin Gräßlin

graesslin accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R278 KWindowSystem

REVISION DETAIL
  https://phabricator.kde.org/D4141

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: subdiff, graesslin
Cc: #frameworks

[Differential] [Closed] D3690: Generate an instance with KSharedConfig::Ptr for singleton and arg

2017-01-06 Thread Martin Gräßlin

This revision was automatically updated to reflect the committed changes.
Closed by commit R237:e0db24223622: Generate an instance with 
KSharedConfig::Ptr for singleton and arg (authored by graesslin).

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D3690?vs=9045=9794

REVISION DETAIL
  https://phabricator.kde.org/D3690

AFFECTED FILES
  autotests/kconfig_compiler/CMakeLists.txt
  autotests/kconfig_compiler/kconfigcompiler_test.cpp
  autotests/kconfig_compiler/test8c.cpp.ref
  autotests/kconfig_compiler/test8c.h.ref
  autotests/kconfig_compiler/test8c.kcfg
  autotests/kconfig_compiler/test8c.kcfgc
  autotests/kconfig_compiler/test8main.cpp
  src/kconfig_compiler/kconfig_compiler.cpp

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: graesslin, #frameworks, mdawson, dfaure

[Differential] [Changed Subscribers] D3987: Use nullptr in all Frameworks (just diff in KIO shown here)

2017-01-05 Thread Martin Gräßlin

graesslin added inline comments.

INLINE COMMENTS

> job.h:50
>  KIOCORE_EXPORT QString buildHTMLErrorString(int errorCode, const QString 
> ,
> -const QUrl *reqUrl = 0, int method = -1);
> +const QUrl *reqUrl = nullptr, int method = -1);
>  

Question: is this change API and ABI stable?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D3987

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: kfunk, #frameworks
Cc: graesslin

Re: Enabling Clang's -Wdocumentation in Frameworks...?

2017-01-05 Thread Martin Gräßlin


Am 2017-01-05 20:01, schrieb Kevin Funk:

Heya,

we all like up-to-date apidocs, right.

I was pondering whether it would be feasible to globally activate 
Clang's -
Wdocumentation warning in KDEFrameworkCompilerSettings.cmake in ECM, 
but we're

running into too many issues at the moment, thus I ditched the idea.

In case you don't know -Wdocumentation is a feature from Clang to 
analyze
doxygen-style comments in source code and thus to check for invalid 
apidocs

[1].

Excerpt from my scan today (I just did 
-DCMAKE_CXX_FLAGS=-Wdocumentation) on

all of KF5:

  .../kf5/attica/src/accountbalance.h:78:15: warning: parameter 
'balance' not

found in the function declaration [-Wdocumentation]
  .../kf5//attica/src/accountbalance.h:78:21: warning: empty paragraph 
passed

to '@param' command [-Wdocumentation]
  .../kf5//attica/src/activity.h:86:15: warning: parameter 'id' not 
found in

the function declaration [-Wdocumentation]

With -Wdocumentation enabled I get around 1000 of unique compiler 
warnings in

KF5 code; I've uploaded the full log to pastebin for reference:
  https://paste.kde.org/p6r1o1m5j (one year lifetime)

tl;dr: If someone is super bored he could go through those warnings and 
fix

them -- only then we could enabled -Wdocumentation globally...

Would be nice to have though. I've just enabled it for all of 
kdevplatform/

kdevelop.


sounds super useful! If we don't get the doc errors fixed till next 
GCI... could be nice tasks


Cheers
Martin

Re: CI Requirements - Lessons Not Learnt?

2017-01-05 Thread Martin Gräßlin


Am 2017-01-05 11:20, schrieb Ben Cooksley:

On Thu, Jan 5, 2017 at 10:28 PM, Martin Gräßlin
<pri...@martin-graesslin.com> wrote:

Am 2017-01-05 09:44, schrieb Ben Cooksley:


Hi all,

It seems that my previous vocal complaints about system level /
serious impact dependency bumps on the CI system have gone completely
unnoticed by (some) members of our Community.

This was demonstrated earlier this week when components of Plasma
bumped their version requirements for XKBCommon and Appstream-Qt -
without even a thought about notifying Sysadmin or checking which
version the CI had, until their builds broke.

Neither of these is easy to fix at this stage, as the system base is
now too old to receive updates such as these. Base upgrades require a
full rebuild of everything on the CI system, and usually involve
significant additional churn and is a process that must be done
roughly twice a year, depending on dependency bump demands.

Does anyone have any suggestions as to how we may avoid this in the
future?



I have a few questions here:

1) Where is this requirement to check with sysadmins codified? So far 
I was

only aware of dependency freeze.


It's been codified since the PIM Qt 5.6 / WebEngine debacle, where
Sysadmin had to rush delivery of Qt 5.6 to the CI system because the
whole of PIM broke when they started using QtWebEngine. That was
around March/April 2016, my mail archives can't seem to find the exact
thread though.


I'm sorry Ben, but I fear "sending out a mail about an issue with PIM" 
doesn't
qualify as codifying it. Given what we have it looks like this did not 
reach the

target audience. And neither will this thread.

This needs to change the process, the way KDE develops software. It 
needs to be
listed in the release schedule (is not, I checked), maybe reviews need 
to be

acked by release managers, etc.



2) How can we easily check what build.kde.org has? Looking at cmake 
output

is not a sufficient way as it gives me wrong information


If CMake is outputting wrong information, then your CMakeLists.txt
can't make the appropriate decisions as to whether the available
version is suitable, so i'd say you've got bigger problems here that
need to be addressed first.


Cmake feature summary says: "required version >= 0.5" and that's for all 
found
depeendencies. Unfortunately no information at all in the feature 
summary about

the actual version.



In any case, you can see the Docker log of the container being
generated at https://build.kde.org/job/create_ubuntu_slave-ange/


and where do I find this information if I would not have asked in a 
mail?
This is very related to properly codifying and documenting such 
requirements.


You cannot tell people "don't introduce new dependencies", without 
telling them

how to check.




3) What should we do when build.kde.org does not have the requirement?


You have to file a Sysadmin ticket, also tagging the project
'build.kde.org' at the same time.


And then? What's the process then? How long do we have to expect this to 
go?
Would it allow to block a finished feature or an important bug fix? 
Would we be

forced to write ifdef hackery?

Sorry, but I'm not thrilled by this process.

What matters to me is getting out good software to our users. And for 
that I have

a hard requirement I have to hit: dependency freeze.





It should be rather obvious that we don't introduce new dependencies 
because

we like to. There is a very important software reason to it.
That's the case for the xkbcommon dependency increase. Should I have 
let the

code broken as it was, expecting half a year of bug reports till
build.kde.org has the base upgraded to Ubuntu 16.04?


That's what #ifdef is for...


I see you volunteer to:
1. write the ifdef
2. adjust the unit test to skip
3. Inform distros that the reported minimum version is wrong, that in 
truth it

  requires a newer version than reported
4. handle all the bug reports related to it

If not, please don't suggest ifdef. We all know that it comes with a 
huge cost.
A cost I decided is too high in this case. After all I had many people 
complain

about it and you can imagine how annoyed I am about the broken build.

If it were as simple as an ifdef, we would have done it, wouldn't we?





If I have to degrade the quality of the product for serving the CI, I 
and
all users have a problem. And this is currently the only alternative. 
The

quality of our product is highly at risk as our changes are no longer
compile tested. This is a huge problem for the release of Plasma 5.9. 
On the
other hand I cannot revert the dependency change as that would break 
tests
or introduce the broken code again. So actually we are caught between 
a hard

and a rock place.

When I increased the dependency I had the dependency freeze of Plasma 
5.9 in
mind. That's the one target I have to hit from release process 
currently.
Also I had to consider a social aspect here. I asked xkbcommo

Re: CI Requirements - Lessons Not Learnt?

2017-01-05 Thread Martin Gräßlin


Sorry picked wrong from address

Am 2017-01-05 10:28, schrieb Martin Gräßlin:

Am 2017-01-05 09:44, schrieb Ben Cooksley:

Hi all,

It seems that my previous vocal complaints about system level /
serious impact dependency bumps on the CI system have gone completely
unnoticed by (some) members of our Community.

This was demonstrated earlier this week when components of Plasma
bumped their version requirements for XKBCommon and Appstream-Qt -
without even a thought about notifying Sysadmin or checking which
version the CI had, until their builds broke.

Neither of these is easy to fix at this stage, as the system base is
now too old to receive updates such as these. Base upgrades require a
full rebuild of everything on the CI system, and usually involve
significant additional churn and is a process that must be done
roughly twice a year, depending on dependency bump demands.

Does anyone have any suggestions as to how we may avoid this in the 
future?


I have a few questions here:

1) Where is this requirement to check with sysadmins codified? So far
I was only aware of dependency freeze.
2) How can we easily check what build.kde.org has? Looking at cmake
output is not a sufficient way as it gives me wrong information
3) What should we do when build.kde.org does not have the requirement?

It should be rather obvious that we don't introduce new dependencies
because we like to. There is a very important software reason to it.
That's the case for the xkbcommon dependency increase. Should I have
let the code broken as it was, expecting half a year of bug reports
till build.kde.org has the base upgraded to Ubuntu 16.04?

If I have to degrade the quality of the product for serving the CI, I
and all users have a problem. And this is currently the only
alternative. The quality of our product is highly at risk as our
changes are no longer compile tested. This is a huge problem for the
release of Plasma 5.9. On the other hand I cannot revert the
dependency change as that would break tests or introduce the broken
code again. So actually we are caught between a hard and a rock place.

When I increased the dependency I had the dependency freeze of Plasma
5.9 in mind. That's the one target I have to hit from release process
currently. Also I had to consider a social aspect here. I asked
xkbcommon devs to do the release. I would have feeled ashamed if we
asked for the release and then don't use it. For me it was from a
social point of view a very high requirement to ship with the
dependency in the next release after xkbcommon release.

If we have to wait an arbitrary time till build.kde.org has upgraded
the base, maybe the choice of the base is not sufficient. E.g. I asked
openSUSE about this dependency weeks ago. Actually a few days after
xkbcommon had the release and it was already shipped in tumbleweed.
Similar for Mesa 13 which I'm also eagerly waiting for build.kde.org
to fetch it.

Cheers
Martin

Re: Review Request 129648: New widget: tooltip that contains another widget

2017-01-01 Thread Martin Gräßlin


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129648/#review101699
---



Looks good to me. Using the transientParent's qscreen was what I meant.

- Martin Gräßlin


On Dec. 30, 2016, 11:27 a.m., Elvis Angelaccio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129648/
> ---
> 
> (Updated Dec. 30, 2016, 11:27 a.m.)
> 
> 
> Review request for KDE Frameworks, Ben Cooksley, Christoph Feck, and Martin 
> Gräßlin.
> 
> 
> Repository: kwidgetsaddons
> 
> 
> Description
> ---
> 
> This new widget is based on the KToolTip code that is duplicated across 
> multiple products: at least Dolphin, systemsettings, kinfocenter, 
> ktp-contact-list.
> 
> Rationale: with a single class in frameworks, it will be possible to apply 
> features/fixes only once. See for example the comments in 
> https://phabricator.kde.org/D3112
> 
> A new feature that the old code doesn't have is the delayed hide: this makes 
> it possible to actually use the widget shown in the tooltip.
> 
> 
> Diffs
> -
> 
>   autotests/CMakeLists.txt ffcce3a046ec98b07c4677578f6bc997de1ef16b 
>   autotests/ktooltipwidgettest.h PRE-CREATION 
>   autotests/ktooltipwidgettest.cpp PRE-CREATION 
>   src/CMakeLists.txt de0a8d965f1541d5ffeec93d1aa06600b0b9c138 
>   src/ktooltipwidget.h PRE-CREATION 
>   src/ktooltipwidget.cpp PRE-CREATION 
>   tests/CMakeLists.txt fcb348b1ae9d4270468c3f9003c5ba5f3903db84 
>   tests/ktooltipwidget_test.h PRE-CREATION 
>   tests/ktooltipwidget_test.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/129648/diff/
> 
> 
> Testing
> ---
> 
> Manual test works both in X11 and Wayland. Unit tests pass.
> 
> Ported Dolphin locally to this new class, everything seems to work (and this 
> will fix bug [#352276](https://bugs.kde.org/show_bug.cgi?id=352276) and 
> [#371223](https://bugs.kde.org/show_bug.cgi?id=371223)).
> 
> 
> Thanks,
> 
> Elvis Angelaccio
> 
>

Re: To C++11 or not?

2016-12-30 Thread Martin Gräßlin

Am 2016-12-30 15:25, schrieb David Faure:

On vendredi 30 décembre 2016 12:56:09 CET Albert Astals Cid wrote:
El divendres, 30 de desembre de 2016, a les 12:24:38 CET, Luigi 
Toscano va

escriure:
> Il 30 dicembre 2016 10:30:22 CET, Kevin Funk  ha scritto:
> >Following-up on this thread, since I saw some more discussion about
> >C++11
> >
> >usage in this RR:
> >  https://git.reviewboard.kde.org/r/129724/
> >
> >Let's put this into some concrete actions, finally. I think we all
> >agree
> >nullptr & override are probably the most apparent issues (since
> >compilers
> >started to warn about both), and *having* 'override' is actually super
> >useful
> >for preventing programmer faults.
> >
> >Let's just speak about allowing nullptr & override, allowing the full
> >set of C
> >++11 is *not* feasible. Reason: Lot's of C++11 feature are only
> >available only
> >in MSVC2015, so we'd just be able to support the latest VS. See [2].
> >
> >Looking at [1] I still see we list GCC 4.5 as minimum requirement.
> >That's
> >pretty ancient. 4.5.1 got released Jul 2010 [3]
> >
> >If we raise that to GCC 4.6 (4.6.0 being released Mar 2011), we can use
> >
> >'nullptr' unconditionally. ktexteditor already did that in public
> >headers for
> >quite some time -- no-one complained.
> >
> >If we raise that to GCC 4.7 (4.7.0 being released Mar 2012), we can use
> >
> >'override' unconditionally *.
> >
> >We already use MSVC2012 as min VS dep, this version has full nullptr &
> >override support. I don't see anyone using MSVC2010 for compiling KF5
> >to be
> >honest...
> >
> >Proposal for [1]:
> >- Raise min GCC version to 4.7
> >- Allow to use override unconditionally
> >- Allow to use nullptr unconditonally
> >
> >ACK?
> >
> >
> >PS: I can do the work, I can script the refactoring with clang-tidy.
> >
> >Let's move forward please.
>
> Hi, this is a really good analysis (also for future reference). In order
> to
> complete it, given that the original idea was "follow Qt's requirement",
> and that we increased in time the required version of Qt, what is the
> current status regarding compilers and Qt?

Our min requirement is Qt 5.5 which according to
http://doc.qt.io/qt-5/supported-platforms-and-configurations.html
compiles with GCC 4.6 (Qt 5.6 has the same supported GCC it seems)

Then that's a no-brainer, we can require gcc 4.6 too.

Qt 5.9 (currently "git dev branch") uses override rather than 
Q_DECL_OVERRIDE,

but we're far from requiring 5.9.

This leads to a different proposal:
- Raise min GCC version to 4.6
- Allow to use nullptr unconditionally
- Use Q_DECL_OVERRIDE.

I fully agree that "having 'override' is actually super useful for 
preventing
programmer faults", but that also works if it's spelled out 
Q_DECL_OVERRIDE
and only ineffective for people *using* frameworks on an older system 
with gcc
4.6. It's still effective for all of us who are working on frameworks, 
which

is where the benefit of "override" is.

What's the plan to enforce that? How is build.kde.org checking that we 
don't use override instead of Q_DECL_OVERRIDE?

This is the point I disagree with. We define nice rules and have no 
means at all to ensure that they are enforced. That's something I have 
brought to the attention of this mailing list several times over the 
last years. Unfortunately nothing has changed.

In my opinion we are lying to ourself and to everybody who wants to use 
frameworks if we say "can be compiled with gcc 4.6" when we aren't even 
testing that.

So please let's get the ordering right: ensure that it compiles with gcc 
4.6, then say we support that. The only other option is to say we 
require whatever gcc version build.kde.org is using.

Cheers
Martin

Re: Review Request 129724: [frameworks] Enable -Wsuggest-override for g++ >= 5.0.0

2016-12-30 Thread Martin Gräßlin



> On Dez. 30, 2016, 7:39 vorm., Martin Gräßlin wrote:
> > Is that enabled by default now? I hope not! This is a completely useless 
> > warning for all frameworks (as we are not allowed to use override) and even 
> > more for a legacy code bases. I don't want to have to adjust the cmake in 
> > all projects I maintain to silence this warning again. And even less I want 
> > to spent days adding overrides to legacy code base.
> 
> Laurent Montel wrote:
> We can use Q_DECL_OVERRIDE which is replaced by override when your gcc 
> support it. So There is not a problem to use this flags no ?

I commented on that aspect in the past. We cannot have both: enforce C++11 and 
at the same time keep compatibility to no C++11.


We need to find a real line and not bullshit around with macros.


Either we say C++11 then enable all of it, or say no. But then no earnings 
please.


I'm seriously annoyed by the stupid dance we are doing.


- Martin


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129724/#review101664
---


On Dez. 30, 2016, 12:48 vorm., Albert Astals Cid wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129724/
> ---
> 
> (Updated Dez. 30, 2016, 12:48 vorm.)
> 
> 
> Review request for Build System and KDE Frameworks.
> 
> 
> Repository: extra-cmake-modules
> 
> 
> Description
> ---
> 
> Gives a nice warning about something that should be marked as override but 
> isn't
> 
> 
> Diffs
> -
> 
>   kde-modules/KDEFrameworkCompilerSettings.cmake 038ddc3 
> 
> Diff: https://git.reviewboard.kde.org/r/129724/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Albert Astals Cid
> 
>

Re: Review Request 129724: [frameworks] Enable -Wsuggest-override for g++ >= 5.0.0

2016-12-29 Thread Martin Gräßlin

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129724/#review101664
---

Is that enabled by default now? I hope not! This is a completely useless 
warning for all frameworks (as we are not allowed to use override) and even 
more for a legacy code bases. I don't want to have to adjust the cmake in all 
projects I maintain to silence this warning again. And even less I want to 
spent days adding overrides to legacy code base.

- Martin Gräßlin

On Dec. 30, 2016, 12:48 a.m., Albert Astals Cid wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129724/
> ---
> 
> (Updated Dec. 30, 2016, 12:48 a.m.)
> 
> 
> Review request for Build System and KDE Frameworks.
> 
> 
> Repository: extra-cmake-modules
> 
> 
> Description
> ---
> 
> Gives a nice warning about something that should be marked as override but 
> isn't
> 
> 
> Diffs
> -
> 
>   kde-modules/KDEFrameworkCompilerSettings.cmake 038ddc3 
> 
> Diff: https://git.reviewboard.kde.org/r/129724/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Albert Astals Cid
> 
>

Re: Review Request 129648: New widget: tooltip that contains another widget

2016-12-29 Thread Martin Gräßlin



> On Dec. 29, 2016, 5:01 p.m., Martin Gräßlin wrote:
> > src/ktooltipwidget.cpp, line 101
> > <https://git.reviewboard.kde.org/r/129648/diff/11/?file=488230#file488230line101>
> >
> > this won't work on Wayland, there is no global cursor pos.
> 
> Elvis Angelaccio wrote:
> hmm, not sure what to use instead.
> `rect`? Or maybe `rect.topLeft()` is enough?

Cant you just use the QScreen of the window you place the tooltip on?


- Martin


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129648/#review101652
---


On Dec. 29, 2016, 11:11 a.m., Elvis Angelaccio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129648/
> ---
> 
> (Updated Dec. 29, 2016, 11:11 a.m.)
> 
> 
> Review request for KDE Frameworks, Ben Cooksley, Christoph Feck, and Martin 
> Gräßlin.
> 
> 
> Repository: kwidgetsaddons
> 
> 
> Description
> ---
> 
> This new widget is based on the KToolTip code that is duplicated across 
> multiple products: at least Dolphin, systemsettings, kinfocenter, 
> ktp-contact-list.
> 
> Rationale: with a single class in frameworks, it will be possible to apply 
> features/fixes only once. See for example the comments in 
> https://phabricator.kde.org/D3112
> 
> A new feature that the old code doesn't have is the delayed hide: this makes 
> it possible to actually use the widget shown in the tooltip.
> 
> 
> Diffs
> -
> 
>   autotests/CMakeLists.txt ffcce3a046ec98b07c4677578f6bc997de1ef16b 
>   autotests/ktooltipwidgettest.h PRE-CREATION 
>   autotests/ktooltipwidgettest.cpp PRE-CREATION 
>   src/CMakeLists.txt de0a8d965f1541d5ffeec93d1aa06600b0b9c138 
>   src/ktooltipwidget.h PRE-CREATION 
>   src/ktooltipwidget.cpp PRE-CREATION 
>   tests/CMakeLists.txt fcb348b1ae9d4270468c3f9003c5ba5f3903db84 
>   tests/ktooltipwidget_test.h PRE-CREATION 
>   tests/ktooltipwidget_test.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/129648/diff/
> 
> 
> Testing
> ---
> 
> Manual test works both in X11 and Wayland. Unit tests pass.
> 
> Ported Dolphin locally to this new class, everything seems to work (and this 
> will fix bug [#352276](https://bugs.kde.org/show_bug.cgi?id=352276) and 
> [#371223](https://bugs.kde.org/show_bug.cgi?id=371223)).
> 
> 
> Thanks,
> 
> Elvis Angelaccio
> 
>

[Differential] [Closed] D3689: KGlobalAccel: [runtime] Introduce a KGLOBALACCEL_TEST_MODE env variable

2016-12-29 Thread Martin Gräßlin

This revision was automatically updated to reflect the committed changes.
Closed by commit R268:4ea7687b11ec: [runtime] Introduce a 
KGLOBALACCEL_TEST_MODE env variable (authored by graesslin).

REPOSITORY
  R268 KGlobalAccel

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D3689?vs=9044=9468

REVISION DETAIL
  https://phabricator.kde.org/D3689

AFFECTED FILES
  src/runtime/globalshortcutsregistry.cpp

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: graesslin, #frameworks, aacid
Cc: aacid

Re: Review Request 129648: New widget: tooltip that contains another widget

2016-12-29 Thread Martin Gräßlin


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129648/#review101652
---




src/ktooltipwidget.cpp (line 101)
<https://git.reviewboard.kde.org/r/129648/#comment68089>

this won't work on Wayland, there is no global cursor pos.


- Martin Gräßlin


On Dec. 29, 2016, 11:11 a.m., Elvis Angelaccio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129648/
> ---
> 
> (Updated Dec. 29, 2016, 11:11 a.m.)
> 
> 
> Review request for KDE Frameworks, Ben Cooksley, Christoph Feck, and Martin 
> Gräßlin.
> 
> 
> Repository: kwidgetsaddons
> 
> 
> Description
> ---
> 
> This new widget is based on the KToolTip code that is duplicated across 
> multiple products: at least Dolphin, systemsettings, kinfocenter, 
> ktp-contact-list.
> 
> Rationale: with a single class in frameworks, it will be possible to apply 
> features/fixes only once. See for example the comments in 
> https://phabricator.kde.org/D3112
> 
> A new feature that the old code doesn't have is the delayed hide: this makes 
> it possible to actually use the widget shown in the tooltip.
> 
> 
> Diffs
> -
> 
>   autotests/CMakeLists.txt ffcce3a046ec98b07c4677578f6bc997de1ef16b 
>   autotests/ktooltipwidgettest.h PRE-CREATION 
>   autotests/ktooltipwidgettest.cpp PRE-CREATION 
>   src/CMakeLists.txt de0a8d965f1541d5ffeec93d1aa06600b0b9c138 
>   src/ktooltipwidget.h PRE-CREATION 
>   src/ktooltipwidget.cpp PRE-CREATION 
>   tests/CMakeLists.txt fcb348b1ae9d4270468c3f9003c5ba5f3903db84 
>   tests/ktooltipwidget_test.h PRE-CREATION 
>   tests/ktooltipwidget_test.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/129648/diff/
> 
> 
> Testing
> ---
> 
> Manual test works both in X11 and Wayland. Unit tests pass.
> 
> Ported Dolphin locally to this new class, everything seems to work (and this 
> will fix bug [#352276](https://bugs.kde.org/show_bug.cgi?id=352276) and 
> [#371223](https://bugs.kde.org/show_bug.cgi?id=371223)).
> 
> 
> Thanks,
> 
> Elvis Angelaccio
> 
>

Re: Review Request 129648: New widget: tooltip that contains another widget

2016-12-28 Thread Martin Gräßlin


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129648/#review101637
---




src/ktooltipwidget.cpp (line 98)
<https://git.reviewboard.kde.org/r/129648/#comment68060>

Do not use QDesktopWidget. Most likely the information is wrong. Use 
QScreen instead.



src/ktooltipwidget.cpp (line 146)
<https://git.reviewboard.kde.org/r/129648/#comment68061>

use a QScopedPointer for the d pointer and don't care about manually 
deleting it ;-)


- Martin Gräßlin


On Dec. 21, 2016, 7:43 p.m., Elvis Angelaccio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129648/
> ---
> 
> (Updated Dec. 21, 2016, 7:43 p.m.)
> 
> 
> Review request for KDE Frameworks, Ben Cooksley, Christoph Feck, and Martin 
> Gräßlin.
> 
> 
> Repository: kwidgetsaddons
> 
> 
> Description
> ---
> 
> This new widget is based on the KToolTip code that is duplicated across 
> multiple products: at least Dolphin, systemsettings, kinfocenter, 
> ktp-contact-list.
> 
> Rationale: with a single class in frameworks, it will be possible to apply 
> features/fixes only once. See for example the comments in 
> https://phabricator.kde.org/D3112
> 
> A new feature that the old code doesn't have is the delayed hide: this makes 
> it possible to actually use the widget shown in the tooltip.
> 
> 
> Diffs
> -
> 
>   autotests/CMakeLists.txt ffcce3a046ec98b07c4677578f6bc997de1ef16b 
>   autotests/ktooltipwidgettest.h PRE-CREATION 
>   autotests/ktooltipwidgettest.cpp PRE-CREATION 
>   src/CMakeLists.txt de0a8d965f1541d5ffeec93d1aa06600b0b9c138 
>   src/ktooltipwidget.h PRE-CREATION 
>   src/ktooltipwidget.cpp PRE-CREATION 
>   tests/CMakeLists.txt fcb348b1ae9d4270468c3f9003c5ba5f3903db84 
>   tests/ktooltipwidget_test.h PRE-CREATION 
>   tests/ktooltipwidget_test.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/129648/diff/
> 
> 
> Testing
> ---
> 
> Manual test works both in X11 and Wayland. Unit tests pass.
> 
> Ported Dolphin locally to this new class, everything seems to work (and this 
> will fix bug [#352276](https://bugs.kde.org/show_bug.cgi?id=352276) and 
> [#371223](https://bugs.kde.org/show_bug.cgi?id=371223)).
> 
> 
> Thanks,
> 
> Elvis Angelaccio
> 
>

Re: Review Request 126291: initial implementation of a platform plugin for OS X (WIP)

2016-12-27 Thread Martin Gräßlin


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126291/#review101602
---




src/kwindowsystem.cpp (lines 728 - 732)
<https://git.reviewboard.kde.org/r/126291/#comment68017>

why did you ifdef the section? It's just a runtime switch not depending on 
any platform specific libraries like the X11 case.



src/platforms/osx/kwindowinfo.mm (line 33)
<https://git.reviewboard.kde.org/r/126291/#comment68020>

what's "Ext"?



src/platforms/osx/kwindowinfo.mm (line 36)
<https://git.reviewboard.kde.org/r/126291/#comment68018>

?



src/platforms/osx/kwindowinfo.mm (lines 38 - 40)
<https://git.reviewboard.kde.org/r/126291/#comment68019>

why are there both public and private variables? For a d-ptr class I don't 
really understand this



src/platforms/osx/kwindowinfo.mm (lines 273 - 276)
<https://git.reviewboard.kde.org/r/126291/#comment68021>

?



src/platforms/osx/kwindowinfo.mm (line 336)
<https://git.reviewboard.kde.org/r/126291/#comment68022>

QByteArrayLiteral



src/platforms/osx/kwindowinfo.mm (line 346)
<https://git.reviewboard.kde.org/r/126291/#comment68023>

QByteArrayLiteral



src/platforms/osx/kwindowinfo.mm (line 349)
<https://git.reviewboard.kde.org/r/126291/#comment68025>

I'm pretty sure the NETWM spec is irrelevant on cocoa



src/platforms/osx/kwindowinfo_p_cocoa.h (line 74)
<https://git.reviewboard.kde.org/r/126291/#comment68026>

wtf is that?



src/platforms/osx/kwindowsystem.cpp (lines 47 - 53)
<https://git.reviewboard.kde.org/r/126291/#comment68027>

?



src/platforms/osx/kwindowsystem_mac_p.h (line 24)
<https://git.reviewboard.kde.org/r/126291/#comment68028>

do we really need this? That's way too much ifedery for me. Either we have 
that feature or not. Have finished things I don't want to see in our code.



src/platforms/osx/kwindowsystem_macobjc.mm (lines 63 - 65)
<https://git.reviewboard.kde.org/r/126291/#comment68029>

?



src/platforms/osx/kwindowsystem_macobjc.mm (line 104)
<https://git.reviewboard.kde.org/r/126291/#comment68032>

is there a possibility that Qt does not use Cocoa?



src/platforms/osx/kwindowsystem_macobjc.mm (line 108)
<https://git.reviewboard.kde.org/r/126291/#comment68030>

?



src/platforms/osx/kwindowsystem_macobjc.mm (lines 111 - 113)
<https://git.reviewboard.kde.org/r/126291/#comment68031>

?



src/platforms/osx/kwindowsystem_macobjc.mm (lines 137 - 138)
<https://git.reviewboard.kde.org/r/126291/#comment68033>

?



src/platforms/osx/kwindowsystem_macobjc.mm (lines 286 - 295)
<https://git.reviewboard.kde.org/r/126291/#comment68034>

?



src/platforms/osx/kwindowsystem_macobjc.mm (lines 301 - 324)
<https://git.reviewboard.kde.org/r/126291/#comment68035>

?



src/platforms/osx/kwindowsystem_macobjc.mm (line 518)
<https://git.reviewboard.kde.org/r/126291/#comment68036>

?



src/platforms/osx/plugin.h (line 2)
<https://git.reviewboard.kde.org/r/126291/#comment68037>

I'm certainly not the author of this header file



src/platforms/osx/plugin.cpp (line 2)
<https://git.reviewboard.kde.org/r/126291/#comment68038>

Also not the author of this file.


- Martin Gräßlin


On Dec. 27, 2016, 5 p.m., René J.V. Bertin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126291/
> ---
> 
> (Updated Dec. 27, 2016, 5 p.m.)
> 
> 
> Review request for KDE Software on Mac OS X and KDE Frameworks.
> 
> 
> Repository: kwindowsystem
> 
> 
> Description
> ---
> 
> KWindowSystem has been lacking a platform plugin for OS X. This RR presents a 
> "backport" of the modified KDE4 KWindowSystem implementation that has been 
> used in the MacPorts kdelibs4 port for the last 2 or 3 (or more) years.
> 
> I have made some initial steps to remove deprecated Carbon API calls, but 
> this is clearly a work in progress.
> 
> Open questions include
> - is there any justification to run an event handler (or Cocoa observer) to 
> keep track of running applications, possibly even listing all their open 
> windows?
> - is there any use for the Qt event listener framework (cf. the 
> NETEventFilter in the X11 plugin)? I haven't yet had time to try to figure 
> out what this "could be good for", and am very open to suggestions in this 
> departments.
> - one application for such an event filter would be a listener that catches 
> the opening and closing of all windows by the running process, and keeps 
> track of their

Re: Review Request 129706: Don't focus progress windows

2016-12-27 Thread Martin Gräßlin


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129706/#review101604
---


Ship it!




Ship It!

- Martin Gräßlin


On Dec. 27, 2016, 1:14 p.m., Martin Tobias Holmedahl Sandsmark wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129706/
> ---
> 
> (Updated Dec. 27, 2016, 1:14 p.m.)
> 
> 
> Review request for KDE Frameworks, Albert Astals Cid, David Faure, and Martin 
> Gräßlin.
> 
> 
> Bugs: 333934
> https://bugs.kde.org/show_bug.cgi?id=333934
> 
> 
> Repository: kjobwidgets
> 
> 
> Description
> ---
> 
> Show the job tracking widget without stealing focus, e. g. from Kate
>  when saving to FTP. This is especially important since there's a 0.5s
>  delay before the window is shown so people often start interacting with
>  the original window when the job progress is shown.
> 
> 
> Diffs
> -
> 
>   src/kwidgetjobtracker.cpp 585867a 
> 
> Diff: https://git.reviewboard.kde.org/r/129706/diff/
> 
> 
> Testing
> ---
> 
> Job progress widgets don't steal focus anymore.
> 
> 
> Thanks,
> 
> Martin Tobias Holmedahl Sandsmark
> 
>

[Differential] [Closed] D3812: Deprecate Plasma::Package API in PluginLoader

2016-12-27 Thread Martin Gräßlin

This revision was automatically updated to reflect the committed changes.
Closed by commit R242:4798310ed788: Deprecate Plasma::Package API in 
PluginLoader (authored by graesslin).

REPOSITORY
  R242 Plasma Frameworks

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D3812?vs=9366=9395

REVISION DETAIL
  https://phabricator.kde.org/D3812

AFFECTED FILES
  src/plasma/pluginloader.cpp
  src/plasma/pluginloader.h

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: graesslin, #plasma, #frameworks, mart
Cc: plasma-devel, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas

Re: Review Request 129706: Don't focus progress windows

2016-12-26 Thread Martin Gräßlin


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129706/#review101581
---



Use QWidget::setAttribute with Qt::WA_ShowWithoutActivating

- Martin Gräßlin


On Dec. 26, 2016, 4:15 p.m., Martin Tobias Holmedahl Sandsmark wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129706/
> ---
> 
> (Updated Dec. 26, 2016, 4:15 p.m.)
> 
> 
> Review request for KDE Frameworks, Albert Astals Cid, David Faure, and Martin 
> Gräßlin.
> 
> 
> Bugs: 333934
> https://bugs.kde.org/show_bug.cgi?id=333934
> 
> 
> Repository: kjobwidgets
> 
> 
> Description
> ---
> 
> If there is already a window with focus, re-set focus to that so we
>  don't steal focus. This is especially important since there's a 0.5s
>  delay before the window is shown so people often start interacting with
>  the original window when this is shown.
> 
> 
> Diffs
> -
> 
>   src/kwidgetjobtracker.cpp 585867a 
> 
> Diff: https://git.reviewboard.kde.org/r/129706/diff/
> 
> 
> Testing
> ---
> 
> Job progress widgets don't steal focus anymore.
> 
> 
> Thanks,
> 
> Martin Tobias Holmedahl Sandsmark
> 
>

[Differential] [Request, 15 lines] D3812: Deprecate Plasma::Package API in PluginLoader

2016-12-26 Thread Martin Gräßlin

graesslin created this revision.
graesslin added reviewers: Plasma, Frameworks.
Restricted Application added a project: Plasma.
Restricted Application added a subscriber: plasma-devel.

REVISION SUMMARY
  As Plasma::Package is deprecated API using Plasma::Package should also
  be deprecated.

REPOSITORY
  R242 Plasma Frameworks

BRANCH
  deprecate-package-in-pluginloader

REVISION DETAIL
  https://phabricator.kde.org/D3812

AFFECTED FILES
  src/plasma/pluginloader.cpp
  src/plasma/pluginloader.h

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: graesslin, #plasma, #frameworks
Cc: plasma-devel, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas

[Differential] [Accepted] D3796: Fix Plasma-QPA filedialog to show wrong directory with QFileDialog::selectUrl()

2016-12-22 Thread Martin Gräßlin

graesslin accepted this revision.
graesslin added a reviewer: graesslin.
This revision is now accepted and ready to land.

REPOSITORY
  R135 Integration for Qt applications in Plasma

BRANCH
  fixFileDialogDirectoryWithSelectUrl

REVISION DETAIL
  https://phabricator.kde.org/D3796

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: kossebau, #frameworks, dfaure, #plasma, graesslin
Cc: plasma-devel, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas

[Differential] [Commented On] D3689: KGlobalAccel: [runtime] Introduce a KGLOBALACCEL_TEST_MODE env variable

2016-12-22 Thread Martin Gräßlin

graesslin added a comment.

  In https://phabricator.kde.org/D3689#70685, @aacid wrote:

  > > We can adjust the shortcuts without having side effects on other tests.
  >
  > Why not just delete the test file on start of each test?

  I like my tests not affecting other tests. If for whatever reason there is 
already a kglobalshortcutsrc in the TEST_DATA_DIR I don't think running a kwin  
(or kglobalaccel) unit test should delete that file.

REVISION DETAIL
  https://phabricator.kde.org/D3689

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: graesslin, #frameworks
Cc: aacid

[Differential] [Commented On] D3689: KGlobalAccel: [runtime] Introduce a KGLOBALACCEL_TEST_MODE env variable

2016-12-21 Thread Martin Gräßlin

graesslin added a comment.


  ping

REVISION DETAIL
  https://phabricator.kde.org/D3689

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: graesslin, #frameworks

Re: Review Request 129665: [KStatusNotifierItem] Restore mnimized window as normal

2016-12-18 Thread Martin Gräßlin



> On Dec. 18, 2016, 9:51 a.m., Martin Gräßlin wrote:
> > src/kstatusnotifieritem.cpp, line 980
> > <https://git.reviewboard.kde.org/r/129665/diff/1/?file=487738#file487738line980>
> >
> > AFAIK show and showNormal is the same. What is the difference here?
> 
> Anthony Fieroni wrote:
> The difference is that showNormal restore window state to normal state 
> i.e. not minimized.
> 1. Minimize window (amarok, kmail or any other KDE app who behave in 
> systray)
> 2. Right click in taskmanager -> close
> 3. Click on icon in systray will show app window (showNormal) rather than 
> show it minized (show)

If you change from show to showNormal it will also reset states like maximized 
or fullscreen. From windowing system point that's bad as the state changes. 
Given that restore as minimized is from windowing system perspective correct. 
Now I agree that restoring to minimized might not make much sense, but then 
this needs to be special cased to ensure that maximized state does not get 
destroyed.


- Martin


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129665/#review101490
---


On Dec. 17, 2016, 4:39 p.m., Anthony Fieroni wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129665/
> ---
> 
> (Updated Dec. 17, 2016, 4:39 p.m.)
> 
> 
> Review request for KDE Frameworks and Martin Gräßlin.
> 
> 
> Repository: knotifications
> 
> 
> Description
> ---
> 
> I think, we want minized window to be shown as normal when it's closed in 
> tray. Why we could want a window to be shown as minimized ?
> 
> 
> Diffs
> -
> 
>   src/kstatusnotifieritem.cpp 1cd5e00 
> 
> Diff: https://git.reviewboard.kde.org/r/129665/diff/
> 
> 
> Testing
> ---
> 
> + Remove deprecated warnings
> 
> 
> Thanks,
> 
> Anthony Fieroni
> 
>

Re: Review Request 129665: [KStatusNotifierItem] Restore mnimized window as normal

2016-12-18 Thread Martin Gräßlin


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129665/#review101490
---




src/kstatusnotifieritem.cpp (line 464)
<https://git.reviewboard.kde.org/r/129665/#comment67935>

please don't mix coding style changes with other changes.



src/kstatusnotifieritem.cpp (line 979)
<https://git.reviewboard.kde.org/r/129665/#comment67936>

AFAIK show and showNormal is the same. What is the difference here?


- Martin Gräßlin


On Dec. 17, 2016, 4:39 p.m., Anthony Fieroni wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129665/
> ---
> 
> (Updated Dec. 17, 2016, 4:39 p.m.)
> 
> 
> Review request for KDE Frameworks and Martin Gräßlin.
> 
> 
> Repository: knotifications
> 
> 
> Description
> ---
> 
> I think, we want minized window to be shown as normal when it's closed in 
> tray. Why we could want a window to be shown as minimized ?
> 
> 
> Diffs
> -
> 
>   src/kstatusnotifieritem.cpp 1cd5e00 
> 
> Diff: https://git.reviewboard.kde.org/r/129665/diff/
> 
> 
> Testing
> ---
> 
> + Remove deprecated warnings
> 
> 
> Thanks,
> 
> Anthony Fieroni
> 
>

[Differential] [Closed] D3636: [kconfig_compiler] Improve documentation about Inherits

2016-12-17 Thread Martin Gräßlin

This revision was automatically updated to reflect the committed changes.
Closed by commit R237:b4206d635444: [kconfig_compiler] Improve documentation 
about Inherits (authored by graesslin).

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D3636?vs=9049=9112

REVISION DETAIL
  https://phabricator.kde.org/D3636

AFFECTED FILES
  src/kconfig_compiler/README.dox

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: graesslin, #frameworks, mdawson, dfaure
Cc: nalvarez

Re: Review Request 129648: New widget: tooltip that contains another widget

2016-12-17 Thread Martin Gräßlin



> On Dec. 14, 2016, 8:21 p.m., Martin Gräßlin wrote:
> > If I see correctly we are losing a feature here: blur behind.
> 
> Elvis Angelaccio wrote:
> Right, I had to drop that because we cannot use KWindowSystem in tier 1. 
> Is there a way to achieve the same feature with Qt only?
> 
> Martin Gräßlin wrote:
> Well there are multiple solutions:
> 
> * don't put it in this framework
> * make this framework a tier 2
> * add a property and let Plasma's platform plugin do the blurring
> 
> Elvis Angelaccio wrote:
> > add a property and let Plasma's platform plugin do the blurring
> 
> Can you expand on this? How would it work? Sounds like the best 
> solution...

sure, we have an example for it. KColorSchemeManager installs a property on the 
app, which is then read by the plasma-integration. In plasma-integration we 
have an event filter which monitors for all windows which get created and then 
install the X11/Wayland property.

In your case it will be even easier. Just set a dynamic property on the QWindow 
(not the QWidget!) to hint a fullscreen blur. Then we can do the respective 
call in plasma-integration.


- Martin


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129648/#review101445
---


On Dec. 13, 2016, 4:45 p.m., Elvis Angelaccio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129648/
> ---
> 
> (Updated Dec. 13, 2016, 4:45 p.m.)
> 
> 
> Review request for KDE Frameworks, Ben Cooksley, Christoph Feck, and Martin 
> Gräßlin.
> 
> 
> Repository: kwidgetsaddons
> 
> 
> Description
> ---
> 
> This new widget is based on the KToolTip code that is duplicated across 
> multiple products: at least Dolphin, systemsettings, kinfocenter, 
> ktp-contact-list.
> 
> Rationale: with a single class in frameworks, it will be possible to apply 
> features/fixes only once. See for example the comments in 
> https://phabricator.kde.org/D3112
> 
> A new feature that the old code doesn't have is the delayed hide: this makes 
> it possible to actually use the widget shown in the tooltip.
> 
> 
> Diffs
> -
> 
>   autotests/CMakeLists.txt ffcce3a046ec98b07c4677578f6bc997de1ef16b 
>   autotests/ktooltipwidgettest.h PRE-CREATION 
>   autotests/ktooltipwidgettest.cpp PRE-CREATION 
>   src/CMakeLists.txt de0a8d965f1541d5ffeec93d1aa06600b0b9c138 
>   src/ktooltipwidget.h PRE-CREATION 
>   src/ktooltipwidget.cpp PRE-CREATION 
>   tests/CMakeLists.txt fcb348b1ae9d4270468c3f9003c5ba5f3903db84 
>   tests/ktooltipwidget_test.h PRE-CREATION 
>   tests/ktooltipwidget_test.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/129648/diff/
> 
> 
> Testing
> ---
> 
> Manual test works both in X11 and Wayland. Unit tests pass.
> 
> Ported Dolphin locally to this new class, everything seems to work (and this 
> will fix bug [#352276](https://bugs.kde.org/show_bug.cgi?id=352276) and 
> [#371223](https://bugs.kde.org/show_bug.cgi?id=371223)).
> 
> 
> Thanks,
> 
> Elvis Angelaccio
> 
>

[Differential] [Updated, 6 lines] D3636: [kconfig_compiler] Improve documentation about Inherits

2016-12-15 Thread Martin Gräßlin

graesslin updated this revision to Diff 9049.
graesslin added a comment.


  Add missing "("

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D3636?vs=9046=9049

BRANCH
  inherits-readme-improvement

REVISION DETAIL
  https://phabricator.kde.org/D3636

AFFECTED FILES
  src/kconfig_compiler/README.dox

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: graesslin, #frameworks, dfaure, mdawson
Cc: nalvarez

[Differential] [Updated, 6 lines] D3636: [kconfig_compiler] Improve documentation about Inherits

2016-12-15 Thread Martin Gräßlin

graesslin updated this revision to Diff 9046.
graesslin added a comment.


  Constructor instead of ctor

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D3636?vs=9018=9046

BRANCH
  inherits-readme-improvement

REVISION DETAIL
  https://phabricator.kde.org/D3636

AFFECTED FILES
  src/kconfig_compiler/README.dox

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: graesslin, #frameworks, dfaure, mdawson
Cc: nalvarez

[Differential] [Request, 194 lines] D3690: Generate an instance with KSharedConfig::Ptr for singleton and arg

2016-12-15 Thread Martin Gräßlin

graesslin created this revision.
graesslin added reviewers: Frameworks, dfaure, mdawson.

REVISION SUMMARY
  In case a kcfg with arg="true" was used and singleton the static
  instance method only accepted a QString config name. This made it
  impossible to combine a singleton config with an already existing and
  open KSharedConfig::Ptr.
  
  With this change an overloaded instance method is added which takes a
  KSharedConfig::Ptr as argument. The private ctor, though, only takes a
  KSharedConfig::Ptr and the instance method taking a QString argument
  uses KSharedConfig::openConfig on the config file name.
  
  The change is source-incompatible in the following situation:
  
  - kcfgfile arg="true"
  - Singleton = true
  - Inherits is specified
  
  In this situation the previous revision created an instance method with
  a QString argument and passed that to the parent constructor. This is
  not in accordance with the documentation. Any user of this behavior was
  relying on a bug. With this change now the call to the parent
  constructor carries a KSharedConfigPtr.

TEST PLAN
  kconfigcompiler tests still pass and a config with singleton
  and arg="true" generates the code as I need it

REPOSITORY
  R237 KConfig

BRANCH
  kconfig-change-try2

REVISION DETAIL
  https://phabricator.kde.org/D3690

AFFECTED FILES
  autotests/kconfig_compiler/CMakeLists.txt
  autotests/kconfig_compiler/kconfigcompiler_test.cpp
  autotests/kconfig_compiler/test8c.cpp.ref
  autotests/kconfig_compiler/test8c.h.ref
  autotests/kconfig_compiler/test8c.kcfg
  autotests/kconfig_compiler/test8c.kcfgc
  autotests/kconfig_compiler/test8main.cpp
  src/kconfig_compiler/kconfig_compiler.cpp

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: graesslin, #frameworks, dfaure, mdawson

[Differential] [Updated] D3689: KGlobalAccel: [runtime] Introduce a KGLOBALACCEL_TEST_MODE env variable

2016-12-15 Thread Martin Gräßlin

graesslin retitled this revision from "[runtime] Introduce a 
KGLOBALACCEL_TEST_MODE env variable" to "KGlobalAccel: [runtime] Introduce a 
KGLOBALACCEL_TEST_MODE env variable".

REVISION DETAIL
  https://phabricator.kde.org/D3689

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: graesslin, #frameworks

[Differential] [Request, 2 lines] D3689: [runtime] Introduce a KGLOBALACCEL_TEST_MODE env variable

2016-12-15 Thread Martin Gräßlin

graesslin created this revision.
graesslin added a reviewer: Frameworks.

REVISION SUMMARY
  The idea behind the env variable is to put kgloabalacceld into test mode
  and let it operate on a in-memory KConfig instead of the normal rc file.
  
  From auto tests in KWin we know that the
  QStandardPath::setTestModeEnabled is not sufficient. If there are
  multiple tests interacting with kglobalaccel they interact with each
  other in ways we normally don't want.
  
  With this env variable we could get KWin to have an in-memory kconfig
  and support better testing of the global shortcuts. We can adjust the
  shortcuts without having side effects on other tests.

BRANCH
  kglobalaccel-test-mode

REVISION DETAIL
  https://phabricator.kde.org/D3689

AFFECTED FILES
  src/runtime/globalshortcutsregistry.cpp

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: graesslin, #frameworks

Re: Review Request 129648: New widget: tooltip that contains another widget

2016-12-15 Thread Martin Gräßlin



> On Dec. 14, 2016, 8:21 p.m., Martin Gräßlin wrote:
> > If I see correctly we are losing a feature here: blur behind.
> 
> Elvis Angelaccio wrote:
> Right, I had to drop that because we cannot use KWindowSystem in tier 1. 
> Is there a way to achieve the same feature with Qt only?

Well there are multiple solutions:

* don't put it in this framework
* make this framework a tier 2
* add a property and let Plasma's platform plugin do the blurring


- Martin


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129648/#review101445
---


On Dec. 13, 2016, 4:45 p.m., Elvis Angelaccio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129648/
> ---
> 
> (Updated Dec. 13, 2016, 4:45 p.m.)
> 
> 
> Review request for KDE Frameworks, Ben Cooksley, Christoph Feck, and Martin 
> Gräßlin.
> 
> 
> Repository: kwidgetsaddons
> 
> 
> Description
> ---
> 
> This new widget is based on the KToolTip code that is duplicated across 
> multiple products: at least Dolphin, systemsettings, kinfocenter, 
> ktp-contact-list.
> 
> Rationale: with a single class in frameworks, it will be possible to apply 
> features/fixes only once. See for example the comments in 
> https://phabricator.kde.org/D3112
> 
> A new feature that the old code doesn't have is the delayed hide: this makes 
> it possible to actually use the widget shown in the tooltip.
> 
> 
> Diffs
> -
> 
>   autotests/CMakeLists.txt ffcce3a046ec98b07c4677578f6bc997de1ef16b 
>   autotests/ktooltipwidgettest.h PRE-CREATION 
>   autotests/ktooltipwidgettest.cpp PRE-CREATION 
>   src/CMakeLists.txt de0a8d965f1541d5ffeec93d1aa06600b0b9c138 
>   src/ktooltipwidget.h PRE-CREATION 
>   src/ktooltipwidget.cpp PRE-CREATION 
>   tests/CMakeLists.txt fcb348b1ae9d4270468c3f9003c5ba5f3903db84 
>   tests/ktooltipwidget_test.h PRE-CREATION 
>   tests/ktooltipwidget_test.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/129648/diff/
> 
> 
> Testing
> ---
> 
> Manual test works both in X11 and Wayland. Unit tests pass.
> 
> Ported Dolphin locally to this new class, everything seems to work (and this 
> will fix bug [#352276](https://bugs.kde.org/show_bug.cgi?id=352276) and 
> [#371223](https://bugs.kde.org/show_bug.cgi?id=371223)).
> 
> 
> Thanks,
> 
> Elvis Angelaccio
> 
>

Re: Review Request 129260: Add find module for QtPlatformSupport

2016-12-15 Thread Martin Gräßlin



> On Oct. 27, 2016, 7:39 a.m., Martin Gräßlin wrote:
> > Due to https://git.reviewboard.kde.org/r/129268/ I'm giving a -2 to adding 
> > to ECM
> 
> David Edmundson wrote:
> To this specific patch, or the concept in general?

To this specific patch


- Martin


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129260/#review100323
---


On Oct. 26, 2016, 11:17 a.m., Kai Uwe Broulik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129260/
> ---
> 
> (Updated Oct. 26, 2016, 11:17 a.m.)
> 
> 
> Review request for KDE Frameworks, Alex Merry and Martin Gräßlin.
> 
> 
> Repository: extra-cmake-modules
> 
> 
> Description
> ---
> 
> Comes from KWin and will eventually be used in Plasma Integration, hence 
> moving it to extra-cmake-modules.
> 
> 
> Diffs
> -
> 
>   find-modules/FindQt5PlatformSupport.cmake PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/129260/diff/
> 
> 
> Testing
> ---
> 
> Removed it from KWin, built KWin, worked.
> 
> Built plasma-integration with QDBusMenu private stuff, worked, although 
> includes there sometimes omit the QtPlatformSupport/ prefix but this is an 
> upstream issue since it works with the files Kwin includes.
> 
> 
> Thanks,
> 
> Kai Uwe Broulik
> 
>

Re: Review Request 129648: New widget: tooltip that contains another widget

2016-12-14 Thread Martin Gräßlin


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129648/#review101445
---



If I see correctly we are losing a feature here: blur behind.


src/ktooltipwidget.cpp (lines 8 - 11)
<https://git.reviewboard.kde.org/r/129648/#comment67922>

Should we update the license header to the LGPLv2.1+ as in 
https://community.kde.org/Policies/Licensing_Policy#LGPL_Header

Obviously applies for other files as well.


- Martin Gräßlin


On Dec. 13, 2016, 4:45 p.m., Elvis Angelaccio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129648/
> ---
> 
> (Updated Dec. 13, 2016, 4:45 p.m.)
> 
> 
> Review request for KDE Frameworks, Ben Cooksley, Christoph Feck, and Martin 
> Gräßlin.
> 
> 
> Repository: kwidgetsaddons
> 
> 
> Description
> ---
> 
> This new widget is based on the KToolTip code that is duplicated across 
> multiple products: at least Dolphin, systemsettings, kinfocenter, 
> ktp-contact-list.
> 
> Rationale: with a single class in frameworks, it will be possible to apply 
> features/fixes only once. See for example the comments in 
> https://phabricator.kde.org/D3112
> 
> A new feature that the old code doesn't have is the delayed hide: this makes 
> it possible to actually use the widget shown in the tooltip.
> 
> 
> Diffs
> -
> 
>   autotests/CMakeLists.txt ffcce3a046ec98b07c4677578f6bc997de1ef16b 
>   autotests/ktooltipwidgettest.h PRE-CREATION 
>   autotests/ktooltipwidgettest.cpp PRE-CREATION 
>   src/CMakeLists.txt de0a8d965f1541d5ffeec93d1aa06600b0b9c138 
>   src/ktooltipwidget.h PRE-CREATION 
>   src/ktooltipwidget.cpp PRE-CREATION 
>   tests/CMakeLists.txt fcb348b1ae9d4270468c3f9003c5ba5f3903db84 
>   tests/ktooltipwidget_test.h PRE-CREATION 
>   tests/ktooltipwidget_test.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/129648/diff/
> 
> 
> Testing
> ---
> 
> Manual test works both in X11 and Wayland. Unit tests pass.
> 
> Ported Dolphin locally to this new class, everything seems to work (and this 
> will fix bug [#352276](https://bugs.kde.org/show_bug.cgi?id=352276) and 
> [#371223](https://bugs.kde.org/show_bug.cgi?id=371223)).
> 
> 
> Thanks,
> 
> Elvis Angelaccio
> 
>

[Differential] [Updated, 5 lines] D3636: [kconfig_compiler] Improve documentation about Inherits

2016-12-14 Thread Martin Gräßlin

graesslin updated this revision to Diff 9018.
graesslin added a comment.


  - fix typos
  - default instead of empty
  - add information about what the ctor is for
  - refer to KConfigSkeleton documentation

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D3636?vs=8912=9018

BRANCH
  inherits-readme-improvement

REVISION DETAIL
  https://phabricator.kde.org/D3636

AFFECTED FILES
  src/kconfig_compiler/README.dox

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: graesslin, #frameworks, dfaure, mdawson
Cc: nalvarez

[Differential] [Request, 3 lines] D3636: [kconfig_compiler] Improve documentation about Inherits

2016-12-10 Thread Martin Gräßlin

graesslin created this revision.
graesslin added reviewers: Frameworks, dfaure.

REVISION SUMMARY
  Better specify the requirements the parent class needs to have.
  KConfigCompiler generates different variants of ctors taking either:
  
  - a QStringLiteral argument (name set in 
  - a KSharedConfig::Ptr argument (arg="true" in )
  - no argument (Inherits=true in kcfgc and no )
  
  In order to have Inherits generate compiling code in all cases the
  parent class needs to provide accessible ctors.
  
  This change updates the docuementation to reflect this.

REPOSITORY
  R237 KConfig

BRANCH
  inherits-readme-improvement

REVISION DETAIL
  https://phabricator.kde.org/D3636

AFFECTED FILES
  src/kconfig_compiler/README.dox

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: graesslin, #frameworks, dfaure

Re: Review Request 129607: Display Application version in About dialog header

2016-12-03 Thread Martin Gräßlin



> On Dec. 3, 2016, 2:29 p.m., Sebastian Kügler wrote:
> > I understand the problem, but I don't like the duplication you're 
> > introducing with this patch: The version number is now on the screen two 
> > times when you open the versions tab, and also, why have a versions tab, 
> > when the version is already displayed.
> 
> Luigi Toscano wrote:
> Change "Version" tab to "Environment"?
> Merge Version into the main info tab (even if I have the feeling that 
> this we discussed)?
> 
> Martin Gräßlin wrote:
> I don't like the name "Environment". I would expect the env variables 
> there
> 
> Luigi Toscano wrote:
> Sure, there are other options. "System details"? "Libraries" (and we 
> could put the poppler version for Okular, for example)? ...

> and we could put the poppler version for Okular

yeah, that was one of the ideas for having an API to adjust the version 
information.

I don't have a good idea for the name of the tab.


- Martin


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129607/#review101235
---


On Dec. 3, 2016, 2:39 p.m., Jean-Baptiste Mardelle wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129607/
> ---
> 
> (Updated Dec. 3, 2016, 2:39 p.m.)
> 
> 
> Review request for KDE Frameworks, KDE Usability and Sebastian Kügler.
> 
> 
> Repository: kxmlgui
> 
> 
> Description
> ---
> 
> Last year, a change in kaboutapplicationdialog (review 124133) moved the 
> application version in a separate tab. However for applications, it is really 
> annoying (several Kdenlive users complained and I fully agree) to not have 
> this information clearly displayed as soon as the About dialog is displayed. 
> I understand the interest of having a separate version tab displaying the 
> various underlying libraries used, but I propose to display again the 
> application version in the About dialog header (see screenshot).
> 
> 
> Diffs
> -
> 
>   src/kaboutapplicationdialog.cpp 156410e 
> 
> Diff: https://git.reviewboard.kde.org/r/129607/diff/
> 
> 
> Testing
> ---
> 
> Reverts a recent change, tested and works.
> 
> 
> File Attachments
> 
> 
> About dialog with version
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2016/12/03/c02f4767-2a74-4c25-8599-de410678867e__about.png
> Updated patch
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2016/12/03/ee13cb47-2162-4be7-8255-3dce7f470c2b__about2.png
> 
> 
> Thanks,
> 
> Jean-Baptiste Mardelle
> 
>

Re: Review Request 129607: Display Application version in About dialog header

2016-12-03 Thread Martin Gräßlin



> On Dec. 3, 2016, 2:29 p.m., Sebastian Kügler wrote:
> > I understand the problem, but I don't like the duplication you're 
> > introducing with this patch: The version number is now on the screen two 
> > times when you open the versions tab, and also, why have a versions tab, 
> > when the version is already displayed.
> 
> Luigi Toscano wrote:
> Change "Version" tab to "Environment"?
> Merge Version into the main info tab (even if I have the feeling that 
> this we discussed)?

I don't like the name "Environment". I would expect the env variables there


- Martin


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129607/#review101235
---


On Dec. 3, 2016, 2:39 p.m., Jean-Baptiste Mardelle wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129607/
> ---
> 
> (Updated Dec. 3, 2016, 2:39 p.m.)
> 
> 
> Review request for KDE Frameworks, KDE Usability and Sebastian Kügler.
> 
> 
> Repository: kxmlgui
> 
> 
> Description
> ---
> 
> Last year, a change in kaboutapplicationdialog (review 124133) moved the 
> application version in a separate tab. However for applications, it is really 
> annoying (several Kdenlive users complained and I fully agree) to not have 
> this information clearly displayed as soon as the About dialog is displayed. 
> I understand the interest of having a separate version tab displaying the 
> various underlying libraries used, but I propose to display again the 
> application version in the About dialog header (see screenshot).
> 
> 
> Diffs
> -
> 
>   src/kaboutapplicationdialog.cpp 156410e 
> 
> Diff: https://git.reviewboard.kde.org/r/129607/diff/
> 
> 
> Testing
> ---
> 
> Reverts a recent change, tested and works.
> 
> 
> File Attachments
> 
> 
> About dialog with version
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2016/12/03/c02f4767-2a74-4c25-8599-de410678867e__about.png
> Updated patch
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2016/12/03/ee13cb47-2162-4be7-8255-3dce7f470c2b__about2.png
> 
> 
> Thanks,
> 
> Jean-Baptiste Mardelle
> 
>

Re: Review Request 129607: Display Application version in About dialog header

2016-12-03 Thread Martin Gräßlin



> On Dec. 3, 2016, 1:28 p.m., Martin Gräßlin wrote:
> > As the one who did the mentioned change: -1 to this
> > 
> > Please note that this change was done in coordination with the usability 
> > group. So please add our usability experts to this review request.
> 
> Martin Gräßlin wrote:
> I think I should explain a little bit more why I give a -1 to this.
> 
> First of all there was a reason to move the version information into a 
> dedicated tab. If we now see that this was a bad move, then we need to go a 
> step back and re-evaluate why the step was done and what would be a better 
> solution.
> 
> Then what I absolutely dislike in the screenshot is that it lists 
> "Version" twice. From a UI perspective I think that is really bad and 
> confusing. The minimum needed change would be a rename of the Version tab and 
> also remove the Version from the Version tab.
> 
> Furthermore I want to mention that an idea back then was to add API which 
> allows to fine tune the Version tab for the application, to allow adding 
> further information.
> 
> Also to consider here is whether the version information is really that 
> important for all our applications to be that prominent. I see that for your 
> case that's true, but honestly I doubt it. The version information is not 
> that important in general and one click more is not that bad. Others might 
> think that frameworks version is so important that it needs to be prominent 
> and some might say that the Qt version is absolutely important. We cannot add 
> everything there. So is it really that important?
> 
> We have many applications where the version information is hardly of 
> interest. Example would be all applications from Plasma on e.g. KDE Neon. The 
> version number is the same as the operating system. It's not important at 
> all. That's clearly something to consider here.

and please also add sebas to the review. He mostly drove the design of this 
change by sitting next to me while I implemented it and gave valuable feedback.


- Martin


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129607/#review101229
---


On Dec. 3, 2016, 1:29 p.m., Jean-Baptiste Mardelle wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129607/
> ---
> 
> (Updated Dec. 3, 2016, 1:29 p.m.)
> 
> 
> Review request for KDE Frameworks and KDE Usability.
> 
> 
> Repository: kxmlgui
> 
> 
> Description
> ---
> 
> Last year, a change in kaboutapplicationdialog (review 124133) moved the 
> application version in a separate tab. However for applications, it is really 
> annoying (several Kdenlive users complained and I fully agree) to not have 
> this information clearly displayed as soon as the About dialog is displayed. 
> I understand the interest of having a separate version tab displaying the 
> various underlying libraries used, but I propose to display again the 
> application version in the About dialog header (see screenshot).
> 
> 
> Diffs
> -
> 
>   src/kaboutapplicationdialog.cpp 156410e 
> 
> Diff: https://git.reviewboard.kde.org/r/129607/diff/
> 
> 
> Testing
> ---
> 
> Reverts a recent change, tested and works.
> 
> 
> File Attachments
> 
> 
> About dialog with version
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2016/12/03/c02f4767-2a74-4c25-8599-de410678867e__about.png
> 
> 
> Thanks,
> 
> Jean-Baptiste Mardelle
> 
>

Re: Review Request 129607: Display Application version in About dialog header

2016-12-03 Thread Martin Gräßlin

> On Dec. 3, 2016, 1:28 p.m., Martin Gräßlin wrote:
> > As the one who did the mentioned change: -1 to this
> > 
> > Please note that this change was done in coordination with the usability 
> > group. So please add our usability experts to this review request.

I think I should explain a little bit more why I give a -1 to this.

First of all there was a reason to move the version information into a 
dedicated tab. If we now see that this was a bad move, then we need to go a 
step back and re-evaluate why the step was done and what would be a better 
solution.

Then what I absolutely dislike in the screenshot is that it lists "Version" 
twice. From a UI perspective I think that is really bad and confusing. The 
minimum needed change would be a rename of the Version tab and also remove the 
Version from the Version tab.

Furthermore I want to mention that an idea back then was to add API which 
allows to fine tune the Version tab for the application, to allow adding 
further information.

Also to consider here is whether the version information is really that 
important for all our applications to be that prominent. I see that for your 
case that's true, but honestly I doubt it. The version information is not that 
important in general and one click more is not that bad. Others might think 
that frameworks version is so important that it needs to be prominent and some 
might say that the Qt version is absolutely important. We cannot add everything 
there. So is it really that important?

We have many applications where the version information is hardly of interest. 
Example would be all applications from Plasma on e.g. KDE Neon. The version 
number is the same as the operating system. It's not important at all. That's 
clearly something to consider here.

- Martin

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129607/#review101229
---

On Dec. 3, 2016, 1:29 p.m., Jean-Baptiste Mardelle wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129607/
> ---
> 
> (Updated Dec. 3, 2016, 1:29 p.m.)
> 
> 
> Review request for KDE Frameworks and KDE Usability.
> 
> 
> Repository: kxmlgui
> 
> 
> Description
> ---
> 
> Last year, a change in kaboutapplicationdialog (review 124133) moved the 
> application version in a separate tab. However for applications, it is really 
> annoying (several Kdenlive users complained and I fully agree) to not have 
> this information clearly displayed as soon as the About dialog is displayed. 
> I understand the interest of having a separate version tab displaying the 
> various underlying libraries used, but I propose to display again the 
> application version in the About dialog header (see screenshot).
> 
> 
> Diffs
> -
> 
>   src/kaboutapplicationdialog.cpp 156410e 
> 
> Diff: https://git.reviewboard.kde.org/r/129607/diff/
> 
> 
> Testing
> ---
> 
> Reverts a recent change, tested and works.
> 
> 
> File Attachments
> 
> 
> About dialog with version
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2016/12/03/c02f4767-2a74-4c25-8599-de410678867e__about.png
> 
> 
> Thanks,
> 
> Jean-Baptiste Mardelle
> 
>

Re: Review Request 129607: Display Application version in About dialog header

2016-12-03 Thread Martin Gräßlin


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129607/#review101229
---



As the one who did the mentioned change: -1 to this

Please note that this change was done in coordination with the usability group. 
So please add our usability experts to this review request.

- Martin Gräßlin


On Dec. 3, 2016, 12:55 p.m., Jean-Baptiste Mardelle wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129607/
> ---
> 
> (Updated Dec. 3, 2016, 12:55 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kxmlgui
> 
> 
> Description
> ---
> 
> Last year, a change in kaboutapplicationdialog (review 124133) moved the 
> application version in a separate tab. However for applications, it is really 
> annoying (several Kdenlive users complained and I fully agree) to not have 
> this information clearly displayed as soon as the About dialog is displayed. 
> I understand the interest of having a separate version tab displaying the 
> various underlying libraries used, but I propose to display again the 
> application version in the About dialog header (see screenshot).
> 
> 
> Diffs
> -
> 
>   src/kaboutapplicationdialog.cpp 156410e 
> 
> Diff: https://git.reviewboard.kde.org/r/129607/diff/
> 
> 
> Testing
> ---
> 
> Reverts a recent change, tested and works.
> 
> 
> File Attachments
> 
> 
> About dialog with version
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2016/12/03/c02f4767-2a74-4c25-8599-de410678867e__about.png
> 
> 
> Thanks,
> 
> Jean-Baptiste Mardelle
> 
>

[Differential] [Closed] D3386: Generate an instance with KSharedConfig::Ptr for singleton and arg

2016-12-02 Thread Martin Gräßlin

This revision was automatically updated to reflect the committed changes.
Closed by commit R237:cd4e6504dfbd: Generate an instance with 
KSharedConfig::Ptr for singleton and arg (authored by graesslin).

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D3386?vs=8228=8702

REVISION DETAIL
  https://phabricator.kde.org/D3386

AFFECTED FILES
  autotests/kconfig_compiler/CMakeLists.txt
  autotests/kconfig_compiler/kconfigcompiler_test.cpp
  autotests/kconfig_compiler/test8c.cpp.ref
  autotests/kconfig_compiler/test8c.h.ref
  autotests/kconfig_compiler/test8c.kcfg
  autotests/kconfig_compiler/test8c.kcfgc
  autotests/kconfig_compiler/test8main.cpp
  src/kconfig_compiler/kconfig_compiler.cpp

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: graesslin, #frameworks, dfaure, mdawson
Cc: ltoscano, aacid, apol

[Differential] [Updated] D3386: Generate an instance with KSharedConfig::Ptr for singleton and arg

2016-11-30 Thread Martin Gräßlin

graesslin added reviewers: dfaure, mdawson.

REVISION DETAIL
  https://phabricator.kde.org/D3386

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: graesslin, #frameworks, dfaure, mdawson
Cc: apol

[Differential] [Commented On] D3386: Generate an instance with KSharedConfig::Ptr for singleton and arg

2016-11-23 Thread Martin Gräßlin

graesslin added a comment.


  ping!

REVISION DETAIL
  https://phabricator.kde.org/D3386

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: graesslin, #frameworks
Cc: apol

Re: Review Request 129460: Add FreeBSD to metainfo.yaml.

2016-11-20 Thread Martin Gräßlin


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129460/#review100984
---


Ship it!




Ship It!

- Martin Gräßlin


On Nov. 20, 2016, 4:38 p.m., Tobias Berner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129460/
> ---
> 
> (Updated Nov. 20, 2016, 4:38 p.m.)
> 
> 
> Review request for KDE Frameworks and Martin Gräßlin.
> 
> 
> Repository: kwayland
> 
> 
> Description
> ---
> 
> Add FreeBSD to metainfo.yaml.
> 
> 
> Diffs
> -
> 
>   metainfo.yaml ba5d2d0639b2d005411afc48bc6861629e8a2299 
> 
> Diff: https://git.reviewboard.kde.org/r/129460/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Tobias Berner
> 
>

Re: Review Request 129417: Add metainfo.yaml

2016-11-16 Thread Martin Gräßlin


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129417/#review100888
---


Ship it!




yay, great to see these points getting addressed.

- Martin Gräßlin


On Nov. 16, 2016, 6:02 p.m., Heiko Becker wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129417/
> ---
> 
> (Updated Nov. 16, 2016, 6:02 p.m.)
> 
> 
> Review request for KDE Frameworks and Sune Vuorela.
> 
> 
> Repository: prison
> 
> 
> Description
> ---
> 
> Striking one requirement for becoming a framework from the list.
> 
> 
> Diffs
> -
> 
>   metainfo.yaml PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/129417/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Heiko Becker
> 
>

[Differential] [Updated, 194 lines] D3386: Generate an instance with KSharedConfig::Ptr for singleton and arg

2016-11-16 Thread Martin Gräßlin

graesslin updated this revision to Diff 8228.
graesslin added a comment.


  Added a test case

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D3386?vs=8220=8228

BRANCH
  kconfigcompiler-instance-ksharedconfig

REVISION DETAIL
  https://phabricator.kde.org/D3386

AFFECTED FILES
  autotests/kconfig_compiler/CMakeLists.txt
  autotests/kconfig_compiler/kconfigcompiler_test.cpp
  autotests/kconfig_compiler/test8c.cpp.ref
  autotests/kconfig_compiler/test8c.h.ref
  autotests/kconfig_compiler/test8c.kcfg
  autotests/kconfig_compiler/test8c.kcfgc
  autotests/kconfig_compiler/test8main.cpp
  src/kconfig_compiler/kconfig_compiler.cpp

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: graesslin, #frameworks
Cc: apol

[Differential] [Commented On] D3386: Generate an instance with KSharedConfig::Ptr for singleton and arg

2016-11-16 Thread Martin Gräßlin

graesslin added a comment.


  Just checked: the only test with a combination of arg="true" and singleton is 
signals_test_singleton.kcfgc which is used by kconfigcompiler_test_signals.cpp, 
but there is no code comparison in that test.
  
  The only other arg="true" case is test8a.kcfg, but that one is not a 
singleton.

REVISION DETAIL
  https://phabricator.kde.org/D3386

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: graesslin, #frameworks
Cc: apol

[Differential] [Commented On] D3386: Generate an instance with KSharedConfig::Ptr for singleton and arg

2016-11-16 Thread Martin Gräßlin

graesslin added a comment.


  In https://phabricator.kde.org/D3386#63201, @apol wrote:
  
  > How can tests still pass? they compare the generated code.
  
  
  eh no idea. I did a make clean, make, make test in the folder. Maybe there is 
no test for the singleton + arg="true" combination?

REVISION DETAIL
  https://phabricator.kde.org/D3386

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: graesslin, #frameworks
Cc: apol

[Differential] [Request, 33 lines] D3386: Generate an instance with KSharedConfig::Ptr for singleton and arg

2016-11-16 Thread Martin Gräßlin

graesslin created this revision.
graesslin added a reviewer: Frameworks.

REVISION SUMMARY
  In case a kcfg with arg="true" was used and singleton the static
  instance method only accepted a QString config name. This made it
  impossible to combine a singleton config with an already existing and
  open KSharedConfig::Ptr.
  
  With this change an overloaded instance method is added which takes a
  KSharedConfig::Ptr as argument. The private ctor, though, only takes a
  KSharedConfig::Ptr and the instance method taking a QString argument
  uses KSharedConfig::openConfig on the config file name.
  
  This provides full API compatibility and at the same time allows to use
  KSharedConfig in addition to the arg name based variant.

TEST PLAN
  kconfigcompiler tests still pass and a config with singleton
  and arg="true" generates the code as I need it

BRANCH
  kconfigcompiler-instance-ksharedconfig

REVISION DETAIL
  https://phabricator.kde.org/D3386

AFFECTED FILES
  src/kconfig_compiler/kconfig_compiler.cpp

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: graesslin, #frameworks

To C++11 or not?

2016-11-14 Thread Martin Gräßlin


Hi framework devs,

recently we started to see the first patches for frameworks to silence 
warnings for not used features of C++11. In particular to add override 
to methods of inheriting classes.


Now I find this weird from the perspective of our C++ requirements. On 
the one side we say that we are not allowed to use these features to 
still support non-C++11 compilers on the other side we see that 
compilers already generate warnings if you don't use the C++11 features.


In that particular case I do not think that Q_DECL_OVERRIDE is a 
solution. It's an ugly hack and for the frameworks I maintain I gave a 
-2 on the review. I think this needs a general solution of either not 
adding or adding override, but not the Qt hack.


I think this is a sign that we need to move on. We cannot continue with 
the state we are in. It's too much a hassle for developers:

* false-positive compile warnings
* no way to check which features are allowed or not
* no warnings if a not allowed feature is used
* no CI system in place to ensure our rules

Given that I want to suggest that we remove all compiler restrictions 
and allow the full feature set of C++11. If someone still thinks we need 
to support the compilers not supporting C++11, I would like to see a 
plan on how we can improve the developer story, especially how to 
address the last two points in my list above.


Opinions?

Cheers
Martin

Re: Review Request 129385: kwindowsystem add overrides

2016-11-12 Thread Martin Gräßlin


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129385/#review100803
---



I'm against adding the Qt variant. Either we use the C++ variant or none. But 
not this stupid dance of we require C++11 but no we don't

so -2 from my side for this achappro

- Martin Gräßlin


On Nov. 12, 2016, 4:41 nachm., Allen Winter wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129385/
> ---
> 
> (Updated Nov. 12, 2016, 4:41 nachm.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kwindowsystem
> 
> 
> Description
> ---
> 
> Add Q_DECL_OVERRIDES as suggested by -Wsuggest-override
> 
> 
> Diffs
> -
> 
>   autotests/kwindowsystem_threadtest.cpp a142bae 
>   src/kstartupinfo.h 9ed86e4 
>   src/platforms/xcb/kselectionowner.h f9ca7e2 
>   src/platforms/xcb/kxmessages.cpp 0ca6bbc 
> 
> Diff: https://git.reviewboard.kde.org/r/129385/diff/
> 
> 
> Testing
> ---
> 
> all warnings for -Wsuggest-override are gone
> make test passes on the same tests as previously.  fyi: a handful of the same 
> autotests fail for me with or without this patch.
> 
> 
> Thanks,
> 
> Allen Winter
> 
>

Re: Review Request 129362: [KWindowInfo] Add pid()

2016-11-12 Thread Martin Gräßlin


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129362/#review100802
---


Ship it!





autotests/kwindowinfox11test.cpp (line 772)
<https://git.reviewboard.kde.org/r/129362/#comment67653>

Lucky you that Qt is smart


- Martin Gräßlin


On Nov. 11, 2016, 11:35 vorm., Kai Uwe Broulik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129362/
> ---
> 
> (Updated Nov. 11, 2016, 11:35 vorm.)
> 
> 
> Review request for KDE Frameworks and Martin Gräßlin.
> 
> 
> Repository: kwindowsystem
> 
> 
> Description
> ---
> 
> Allows to retrieve the process ID of the window's application if present.
> 
> 
> Diffs
> -
> 
>   autotests/kwindowinfox11test.cpp 09d2837 
>   src/kwindowinfo.h e3dea61 
>   src/kwindowinfo.cpp e40e397 
>   src/kwindowinfo_dummy_p.h 933140d 
>   src/kwindowinfo_p.h 7a4ede5 
>   src/platforms/xcb/kwindowinfo.cpp 3a3fee6 
>   src/platforms/xcb/kwindowinfo_p_x11.h aeb046a 
> 
> Diff: https://git.reviewboard.kde.org/r/129362/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kai Uwe Broulik
> 
>

Re: Review Request 129362: [KWindowInfo] Add pid()

2016-11-10 Thread Martin Gräßlin


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129362/#review100762
---



code looks good now, but please extend autotests/kwindowinfox11test.cpp with a 
test method for pid.

- Martin Gräßlin


On Nov. 10, 2016, 3:30 p.m., Kai Uwe Broulik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129362/
> ---
> 
> (Updated Nov. 10, 2016, 3:30 p.m.)
> 
> 
> Review request for KDE Frameworks and Martin Gräßlin.
> 
> 
> Repository: kwindowsystem
> 
> 
> Description
> ---
> 
> Allows to retrieve the process ID of the window's application if present.
> 
> 
> Diffs
> -
> 
>   src/kwindowinfo.h e3dea61 
>   src/kwindowinfo.cpp e40e397 
>   src/kwindowinfo_dummy_p.h 933140d 
>   src/kwindowinfo_p.h 7a4ede5 
>   src/platforms/xcb/kwindowinfo.cpp 3a3fee6 
>   src/platforms/xcb/kwindowinfo_p_x11.h aeb046a 
> 
> Diff: https://git.reviewboard.kde.org/r/129362/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kai Uwe Broulik
> 
>

Re: Review Request 129327: Expose desktopFileName in KWindowInfo

2016-11-09 Thread Martin Gräßlin



> On Nov. 8, 2016, 6:08 p.m., Aleix Pol Gonzalez wrote:
> > It's weird that it's called `desktopFileName` but it doesn't offer a file 
> > name? I think it should be renamed or fixed to provide the path. In the end 
> > it's an API that then requires the user to do a rather big look-up.

The naming follows the naming in: NETWinInfo, KAboutData and QGuiApplication. 
Naming it differently would be wrong IMHO.


- Martin


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129327/#review100732
---


On Nov. 7, 2016, 7:29 a.m., Martin Gräßlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129327/
> ---
> 
> (Updated Nov. 7, 2016, 7:29 a.m.)
> 
> 
> Review request for KDE Frameworks and Eike Hein.
> 
> 
> Repository: kwindowsystem
> 
> 
> Description
> ---
> 
> This change introduced a new method in KWindowInfo:
> QByteArray KWindowInfo::desktopFileName() const
> 
> It returns the desktop file name of the application if known on the
> given platform. So far only provided on X11 through
> NETWinInfo::desktopFileName.
> 
> 
> Diffs
> -
> 
>   autotests/kwindowinfox11test.cpp 5dfbcfa67d74244122c86433a40a7ed6923fb1ab 
>   src/kwindowinfo.h 5d9799b20d640caa1b1cf9ab4d9dc69b8cceefe3 
>   src/kwindowinfo.cpp 658a0b645797676d4e48585ede3d832333688081 
>   src/kwindowinfo_p.h 45390c06e7b5ad064ea9368ca102b2462a029c06 
>   src/platforms/xcb/kwindowinfo.cpp eca607e18a979439593e05e1da232548d0e7d139 
>   src/platforms/xcb/kwindowinfo_p_x11.h 
> 68805765fd630c2bc7cf0d77be688333b4a363f7 
> 
> Diff: https://git.reviewboard.kde.org/r/129327/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Martin Gräßlin
> 
>

Re: Review Request 129362: [KWindowInfo] Add pid()

2016-11-08 Thread Martin Gräßlin


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129362/#review100731
---



You cannot add a virtual method. You need to do an extension dance as my last 
commit.

- Martin Gräßlin


On Nov. 8, 2016, 12:47 nachm., Kai Uwe Broulik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129362/
> ---
> 
> (Updated Nov. 8, 2016, 12:47 nachm.)
> 
> 
> Review request for KDE Frameworks and Martin Gräßlin.
> 
> 
> Repository: kwindowsystem
> 
> 
> Description
> ---
> 
> Allows to retrieve the process ID of the window's application if present.
> 
> 
> Diffs
> -
> 
>   src/kwindowinfo.h e3dea61 
>   src/kwindowinfo.cpp e40e397 
>   src/kwindowinfo_dummy_p.h 933140d 
>   src/kwindowinfo_p.h 7a4ede5 
>   src/platforms/xcb/kwindowinfo.cpp 3a3fee6 
>   src/platforms/xcb/kwindowinfo_p_x11.h aeb046a 
> 
> Diff: https://git.reviewboard.kde.org/r/129362/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kai Uwe Broulik
> 
>

Re: Review Request 129327: Expose desktopFileName in KWindowInfo

2016-11-06 Thread Martin Gräßlin


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129327/
---

(Updated Nov. 7, 2016, 7:29 a.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks and Eike Hein.


Repository: kwindowsystem


Description
---

This change introduced a new method in KWindowInfo:
QByteArray KWindowInfo::desktopFileName() const

It returns the desktop file name of the application if known on the
given platform. So far only provided on X11 through
NETWinInfo::desktopFileName.


Diffs
-

  autotests/kwindowinfox11test.cpp 5dfbcfa67d74244122c86433a40a7ed6923fb1ab 
  src/kwindowinfo.h 5d9799b20d640caa1b1cf9ab4d9dc69b8cceefe3 
  src/kwindowinfo.cpp 658a0b645797676d4e48585ede3d832333688081 
  src/kwindowinfo_p.h 45390c06e7b5ad064ea9368ca102b2462a029c06 
  src/platforms/xcb/kwindowinfo.cpp eca607e18a979439593e05e1da232548d0e7d139 
  src/platforms/xcb/kwindowinfo_p_x11.h 
68805765fd630c2bc7cf0d77be688333b4a363f7 

Diff: https://git.reviewboard.kde.org/r/129327/diff/


Testing
---


Thanks,

Martin Gräßlin

Review Request 129327: Expose desktopFileName in KWindowInfo

2016-11-04 Thread Martin Gräßlin


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129327/
---

Review request for KDE Frameworks and Eike Hein.


Repository: kwindowsystem


Description
---

This change introduced a new method in KWindowInfo:
QByteArray KWindowInfo::desktopFileName() const

It returns the desktop file name of the application if known on the
given platform. So far only provided on X11 through
NETWinInfo::desktopFileName.


Diffs
-

  autotests/kwindowinfox11test.cpp 5dfbcfa67d74244122c86433a40a7ed6923fb1ab 
  src/kwindowinfo.h 5d9799b20d640caa1b1cf9ab4d9dc69b8cceefe3 
  src/kwindowinfo.cpp 658a0b645797676d4e48585ede3d832333688081 
  src/kwindowinfo_p.h 45390c06e7b5ad064ea9368ca102b2462a029c06 
  src/platforms/xcb/kwindowinfo.cpp eca607e18a979439593e05e1da232548d0e7d139 
  src/platforms/xcb/kwindowinfo_p_x11.h 
68805765fd630c2bc7cf0d77be688333b4a363f7 

Diff: https://git.reviewboard.kde.org/r/129327/diff/


Testing
---


Thanks,

Martin Gräßlin

Re: Review Request 129222: ClipboardUpdater: fix crash on Wayland

2016-10-28 Thread Martin Gräßlin



> On Oct. 28, 2016, 9:23 a.m., David Faure wrote:
> > So we won't be able to use the clipboard at all, on wayland?
> > 
> > This seems to me like a Qt bug.

of course we can use the clipboard on Wayland. I'm happily copy'n'pasting 
between windows here.

What is different on Wayland is that clipboard is not broadcasted as on X11. 
It's only available to the window having keyboard focus. As long as a window 
didn't get keyboard focus, there is no clipboard data.


- Martin


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129222/#review100348
---


On Oct. 27, 2016, 6:27 p.m., Elvis Angelaccio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129222/
> ---
> 
> (Updated Oct. 27, 2016, 6:27 p.m.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Bugs: 370520
> https://bugs.kde.org/show_bug.cgi?id=370520
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> QClipboard::mimeData() can return a nullptr on Wayland. The documentation of 
> the method doesn't say that the pointer can never be null, anyway.
> 
> 
> Diffs
> -
> 
>   src/widgets/clipboardupdater.cpp a459aa777ae5bc328cf8807827ed24ddaf952d6d 
> 
> Diff: https://git.reviewboard.kde.org/r/129222/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Elvis Angelaccio
> 
>

1 2 3 4 5 6 7 8 9 10 >

1 - 100 of 1168 matches

Mail list logo