D26888: work around to fully support the windows backend

[Albert Vaca Cintora]

albertvaka added a comment.


  @vonreth @broulik Can you folks give this a final review? We have been 
building KDE Connect for Windows off a custom branch with this patch, but I 
would like to use official builds of KNotifications so it would be great if we 
can merge it.

REPOSITORY
  R289 KNotifications

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

To: brute4s99, vonreth, broulik, #kde_connect
Cc: albertvaka, meven, kde-frameworks-devel, LeGast00n, cblack, michaelh, 
ngraham, bruns

Re: Update on Status of Gitlab Migration

[Albert Vaca Cintora]

On Sat, Apr 11, 2020 at 11:36 AM Ben Cooksley  wrote:
>
> Good morning Community,
>
> I'm pleased to report that this week we reached a major milestone,
> with all the necessary technical components now being in place on our
> side for our migration to Gitlab to take place.

Regarding this: is the subdomain going to stay invent.kde.org once we
have officially moved? I find it's a bit confusing to use that instead
of gitlab.kde.org

Albert

D27210: add KDEconnect Icons

[Albert Vaca Cintora]

albertvaka added a comment.


  Hey, thanks for working on this!
  
  A "brand" re-design is something that we could really use. One of the things 
that I would like to change from the current icon/logo, though (and that this 
new one doesn't change), is the fact that there is a mobile phone in it.
  
  You can use KDE Connect between two computers, or between two phones 
(although it's not the main development focus at the moment). So I think a more 
generic icon that embodies "syncing" without any specific device would suit us 
better, IMO. Plus I find a bit weird to have a phone app whose icon is... a 
phone.

REPOSITORY
  R266 Breeze Icons

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

To: mbruchert, #vdg, #kde_connect
Cc: albertvaka, nicolasfella, kde-frameworks-devel, LeGast00n, cblack, GB_2, 
michaelh, ngraham, bruns

D26888: work around to fully support the windows backend

[Albert Vaca Cintora]

albertvaka added inline comments.

INLINE COMMENTS

> notifybysnore.cpp:135-139
> +if (id == -1) {
> +emit actionInvoked(0, actionNum);
> +} else {
> +emit actionInvoked(id, actionNum);
> +}

This could become:

  if (id == -1) {
  id = 0
  }
  emit actionInvoked(id, actionNum);

To not duplicate the emit line. Still, it feels hackish.

REPOSITORY
  R289 KNotifications

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

To: brute4s99, vonreth, broulik, #kde_connect
Cc: albertvaka, meven, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
ngraham, bruns

D23694: Add support for sshfs to the fstab backend

[Albert Vaca Cintora]

albertvaka added a comment.


  If it's a problem for kdeconnect mounts to appear there, how can we hide it? 
It's an sshfs mountpoint like any other, only that it is mounted 
programmatically.

REPOSITORY
  R245 Solid

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

To: lbeltrame, bruns, broulik, fvogt, #kde_connect
Cc: albertvaka, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D22580: Notify users when not using KDE_INSTALL_USE_QT_SYS_PATHS about prefix.sh

[Albert Vaca Cintora]

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

REPOSITORY
  R240 Extra CMake Modules

BRANCH
  master

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

To: apol, #frameworks, albertvaka
Cc: albertvaka, kde-frameworks-devel, kde-buildsystem, LeGast00n, sbergeron, 
bencreasy, michaelh, ngraham, bruns

D22580: Notify users when not using KDE_INSTALL_USE_QT_SYS_PATHS about prefix.sh

[Albert Vaca Cintora]

albertvaka added inline comments.

INLINE COMMENTS

> KDEInstallDirs.cmake:721
> +if(NOT KDE_INSTALL_USE_QT_SYS_PATHS)
> +message(NOTICE "Installing in ${CMAKE_INSTALL_PREFIX}, remember set the 
> environment. See ${CMAKE_CURRENT_BINARY_DIR}/prefix.sh")
> +endif()

Installing in `${CMAKE_INSTALL_PREFIX}`. You will need to run `./prefix.sh` to 
set the environment for this app.

?

REPOSITORY
  R240 Extra CMake Modules

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

To: apol, #frameworks
Cc: albertvaka, kde-frameworks-devel, kde-buildsystem, LeGast00n, sbergeron, 
bencreasy, michaelh, ngraham, bruns

D22105: WIP : Fix SFTP Plugin of KIO for Windows

[Albert Vaca Cintora]

albertvaka added inline comments.

INLINE COMMENTS

> brute4s99 wrote in kio_sftp.cpp:2257
> yes! thanks Hannah! 

You can keep QT_STAT_LNK on every platform and remove the ifdef. That's the 
point of the abstraction Qt provides.

For consistency, I would also change the other (S_IFDIR, etc.) to their Qt 
counterparts.

REPOSITORY
  R320 KIO Extras

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

To: brute4s99, albertvaka, vonreth, sredman, sitter, dfaure
Cc: andriusr, kde-frameworks-devel, kfm-devel, fprice, LeGast00n, fbampaloukas, 
alexde, feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, mikesomov

D22105: WIP : Fix SFTP Plugin of KIO for Windows

[Albert Vaca Cintora]

albertvaka added inline comments.

INLINE COMMENTS

> andriusr wrote in kio_sftp.cpp:2257
> I'm not sure what is intended to achieve here, the remote file _can_ be a 
> symlink, and IIRC when we had sftp on dolphin in KDE4 those symlinks were 
> shown as such.

I think what they mean is that if you remove the ifdef it should just work.

REPOSITORY
  R320 KIO Extras

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

To: brute4s99, albertvaka, vonreth, sredman, sitter, dfaure
Cc: andriusr, kde-frameworks-devel, kfm-devel, fprice, LeGast00n, fbampaloukas, 
alexde, feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, mikesomov

D21369: [WIP] Add AbstractContact properties for KContact::PhoneNumber objects

[Albert Vaca Cintora]

albertvaka added a comment.


  If I understand correctly, the only reason to use KContacts here is so we can 
use its PhoneNumber type. We could change it to a QString?

REPOSITORY
  R307 KPeople

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

To: sredman, apol
Cc: albertvaka, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D21661: add snoretoast backend for KNotifications on Windows

[Albert Vaca Cintora]

albertvaka added inline comments.

INLINE COMMENTS

> notifybysnore.h:45
> +QMap> m_notifications;
> +QString program = QStringLiteral("SnoreToast.exe");
> +QLocalServer *server;

Isn't LibSnore abstracting this? Why do we need to call the exe instead of 
using some method in LibSnore?

REPOSITORY
  R289 KNotifications

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

To: brute4s99, broulik, sredman, vonreth, albertvaka
Cc: pino, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D16951: Add mouse button icons

[Albert Vaca Cintora]

albertvaka added a reviewer: KDE Connect.

REPOSITORY
  R266 Breeze Icons

BRANCH
  mouse-buttons (branched from master)

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

To: ndavis, #vdg, ngraham, nicolasfella, #kde_connect
Cc: trickyricky26, abetts, ngraham, rizzitello, nicolasfella, 
kde-frameworks-devel, michaelh, bruns

D16692: A QApplication object needs to be instantiated for kio-kdeconnect to work on KDE Neon

[Albert Vaca Cintora]

albertvaka added a comment.


  In D16692#355068 , @fvogt wrote:
  
  > In D16692#355067 , @albertvaka 
wrote:
  >
  > > In D16692#354990 , @fvogt 
wrote:
  > >
  > > > I'm wondering why this specifically mentions "KDE Neon" both in the 
title and in the commit message.
  > > >
  > > > Is this workaround/fix only needed on neon or is the message wrong?
  > >
  > >
  > > I think only Neon ships frameworks 5.51
  >
  >
  > Why would you think that? 5.51 got released almost a month ago.
  
  
  Of curse I don't know every distro in the planet, but we got like a gazillion 
bug reports and I would say all of them are from Neon users, so this fix was 
quite directed to them.
  
  See: https://bugs.kde.org/show_bug.cgi?id=400178
  
  You can rename it, although it has been merged already.

REPOSITORY
  R224 KDE Connect

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

To: eduisters, #kde_connect, #frameworks, albertvaka
Cc: fvogt, sredman, apol, jriddell, nicolasfella, albertvaka, kdeconnect, 
shivanshukantprasad, skymoore, wistak, dvalencia, rmenezes, julioc, Leptopoda, 
timothyc, jdvr, yannux, Danial0_0, johnq, Pitel, adeen-s, SemperPeritus, 
daniel.z.tg, jeanv, seebauer, bugzy, MayeulC, menasshock, tctara

D16692: A QApplication object needs to be instantiated for kio-kdeconnect to work on KDE Neon

[Albert Vaca Cintora]

albertvaka added a comment.


  In D16692#354990 , @fvogt wrote:
  
  > I'm wondering why this specifically mentions "KDE Neon" both in the title 
and in the commit message.
  >
  > Is this workaround/fix only needed on neon or is the message wrong?
  
  
  I think only Neon ships frameworks 5.51

REPOSITORY
  R224 KDE Connect

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

To: eduisters, #kde_connect, #frameworks, albertvaka
Cc: fvogt, sredman, apol, jriddell, nicolasfella, albertvaka, kdeconnect, 
shivanshukantprasad, skymoore, wistak, dvalencia, rmenezes, julioc, Leptopoda, 
timothyc, jdvr, yannux, Danial0_0, johnq, Pitel, adeen-s, SemperPeritus, 
daniel.z.tg, jeanv, seebauer, bugzy, MayeulC, menasshock, tctara

D16692: A QApplication object needs to be instantiated for kio-kdeconnect to work on KDE Neon

[Albert Vaca Cintora]

This revision was automatically updated to reflect the committed changes.
Closed by commit R224:560e8638e8dd: A QApplication object needs to be 
instantiated for kio-kdeconnect to work on… (authored by eduisters, committed 
by albertvaka).

REPOSITORY
  R224 KDE Connect

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16692?vs=44954=44964

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

AFFECTED FILES
  kio/CMakeLists.txt
  kio/kdeconnect.json
  kio/kdeconnect.protocol
  kio/kiokdeconnect.cpp

To: eduisters, #kde_connect, #frameworks, albertvaka
Cc: sredman, apol, jriddell, nicolasfella, albertvaka, kdeconnect, 
shivanshukantprasad, skymoore, wistak, dvalencia, rmenezes, julioc, Leptopoda, 
timothyc, jdvr, yannux, Danial0_0, johnq, Pitel, adeen-s, SemperPeritus, 
daniel.z.tg, jeanv, seebauer, bugzy, MayeulC, menasshock, tctara

D16189: kio_help: Fix crash in QCoreApplication when accessing help://

[Albert Vaca Cintora]

albertvaka added a comment.


  Old apps that didn't have a QApplication are broken by this version of KIO. 
Definitely a regression to me :/

REPOSITORY
  R241 KIO

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

To: mpyne, #frameworks, sitter, dfaure, broulik
Cc: sredman, albertvaka, broulik, dfaure, kde-frameworks-devel, michaelh, 
ngraham, bruns

D16692: A QApplication object needs to be instantiated for kio-kdeconnect to work on KDE Neon

[Albert Vaca Cintora]

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


  Let's ship this, but I still think we should see if something can be done on 
the KIO side to not break old apps.

REPOSITORY
  R224 KDE Connect

BRANCH
  kde-neon

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

To: eduisters, #kde_connect, #frameworks, albertvaka
Cc: sredman, apol, jriddell, nicolasfella, albertvaka, kdeconnect, 
shivanshukantprasad, skymoore, wistak, dvalencia, rmenezes, julioc, Leptopoda, 
timothyc, jdvr, yannux, Danial0_0, johnq, Pitel, adeen-s, SemperPeritus, 
daniel.z.tg, jeanv, seebauer, bugzy, MayeulC, menasshock, tctara

D16189: kio_help: Fix crash in QCoreApplication when accessing help://

[Albert Vaca Cintora]

albertvaka added a comment.


  As I commented in this similar patch (https://phabricator.kde.org/D16692 ) I 
think this is a regression that should be fixed in KIO.

REPOSITORY
  R241 KIO

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

To: mpyne, #frameworks, sitter, dfaure, broulik
Cc: albertvaka, broulik, dfaure, kde-frameworks-devel, michaelh, ngraham, bruns

D16692: A QApplication object needs to be instantiated for kio-kdeconnect to work on KDE Neon

[Albert Vaca Cintora]

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


  In my opinion this is a regression in KIO and it would be nice to check if it 
can somehow be fixed there: Upgrading KIO should not break existing apps.
  
  Even if we make a release with this fix, it doesn't guarantee all distros 
will pick it up.

INLINE COMMENTS

> kiokdeconnect.cpp:155
>  KIO::UDSEntry entry;
> -entry.insert(KIO::UDSEntry::UDS_NAME, QStringLiteral("files"));
> -entry.insert(KIO::UDSEntry::UDS_DISPLAY_NAME, name);

Also don't change insert to fastInsert, as it requires KIO 5.48 and we want to 
support older than that.

REPOSITORY
  R224 KDE Connect

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

To: eduisters, #kde_connect, #frameworks, albertvaka
Cc: albertvaka, kdeconnect, shivanshukantprasad, skymoore, wistak, dvalencia, 
rmenezes, julioc, Leptopoda, timothyc, jdvr, yannux, Danial0_0, johnq, Pitel, 
adeen-s, SemperPeritus, daniel.z.tg, jeanv, seebauer, bugzy, MayeulC, 
menasshock, tctara, apol

D11683: Make it possible to request a plugin configuration module programatically

[Albert Vaca Cintora]

albertvaka accepted this revision.
albertvaka added inline comments.
This revision is now accepted and ready to land.

INLINE COMMENTS

> kpluginselector.h:214
> + * Shows the configuration dialog for the plugin @p pluginId if it's 
> available
> + */
> +void showConfiguration(const QString );

Remember to add the `@since 5.X` annotation before pushing the change.

REPOSITORY
  R295 KCMUtils

BRANCH
  master

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

To: apol, #frameworks, #kde_connect, albertvaka
Cc: albertvaka, michaelh, ngraham

D6906: Create a "." entry even on empty dirs

[Albert Vaca Cintora]

albertvaka added a comment.


  Mmmm it could be because kio_desktop is a ForwardingSlave, so the entry is 
added first by its forwardee (kio_file I think) and then again by kio_desktop 
itself?
  
  I'm not sure if what I'm saying even makes sense because I don't really know 
how KIO works. Will investigate a bit...

REPOSITORY
  R241 KIO

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

To: albertvaka, #frameworks, elvisangelaccio
Cc: aacid, dfaure

D6906: Create a "." entry even on empty dirs

[Albert Vaca Cintora]

albertvaka added a comment.


  Reverted the patch. Couldn't figure out what is wrong with it though... I 
don't think that "." is being emitted multiple times because of this patch. It 
looks more like if kio_desktop was causing listEntries to be called twice.

REPOSITORY
  R241 KIO

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

To: albertvaka, #frameworks, elvisangelaccio
Cc: aacid, dfaure

D6906: Create a "." entry even on empty dirs

[Albert Vaca Cintora]

This revision was automatically updated to reflect the committed changes.
Closed by commit R241:0ac78aa3fba4: Emit a "." UDSEntry when not present, even 
on empty directories. (authored by albertvaka).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D6906?vs=17194=17207

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

AFFECTED FILES
  src/core/slavebase.cpp

To: albertvaka, #frameworks, elvisangelaccio

D6906: Create a "." entry even on empty dirs

[Albert Vaca Cintora]

albertvaka added a reviewer: elvisangelaccio.

REPOSITORY
  R241 KIO

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

To: albertvaka, #frameworks, elvisangelaccio

D6906: Create a "." entry even on empty dirs

[Albert Vaca Cintora]

albertvaka edited the summary of this revision.

REPOSITORY
  R241 KIO

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

To: albertvaka, #frameworks

D6906: Create a "." entry even on empty dirs

[Albert Vaca Cintora]

albertvaka created this revision.
Restricted Application added a project: Frameworks.

REVISION SUMMARY
  Fixes bug 382046

REPOSITORY
  R241 KIO

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

AFFECTED FILES
  src/core/slavebase.cpp

To: albertvaka, #frameworks

[Differential] [Abandoned] D4663: Allow setting the timeout value.

[Albert Vaca Cintora]

albertvaka abandoned this revision.
albertvaka added a comment.


  I'm discarding this patch and will find an alternative way to solve the 
problem in KDE Connect. I still think, though, that notifications are a way 
better solution to require interaction from the user than SNIs, so I don't 
think I will switch to SNIs.
  
  In https://phabricator.kde.org/D4663#88444, @colomar wrote:
  
  > As long as we can't get there, I agree with Martin that an SNI is a better 
solution than a permanently shown popup notification, as it attracts attention 
without covering anything on the screen.
  
  
  Even without this patch, we already support persistent notifications. Of 
course, they are not implemented as a permanently shown popup: we have a 
notifications menu that groups them, in a similar way to what Android, iOS, 
Gnome, Mac and Windows do. I think it's way easier to use this functionality 
already present in notifications than to implement all the changes you propose 
to SNIs (and that without taking into account that our SNIs would behave 
completely different than in any other desktop).
  
  In https://phabricator.kde.org/D4663#88456, @mck182 wrote:
  
  > Ever wondered why the spec you're referring to
  >  is full of only Gnome's extensions and is hosted on Gnome's
  >  servers? It's because Gnome considers the Galago project,
  >  the actual cross-desktop notifications project, dead.
  
  
  The Galago spec hasn't been updated since 2006. IMHO it's actually a good 
thing that Gnome took the lead here and continued to maintain it: otherwise we 
would have nothing (and no, don't say we would have SNIs... even Windows and 
Mac have deprecated them in favor of persistent notifications!). KNotifications 
has for a long time implemented most of the fetures that were added by Gnome, 
so it's not a Gnome-only thing anymore even if it hasn't moved upstream to 
Freedesktop yet.
  
  I don't see why you guys stick to the idea of using SNIs, when that would 
require a ton of changes to them, when we already have most of what we need in 
the notifications system. Not only it's already implemented, but also it is 
what every major desktop and phone OS out there already use!

REPOSITORY
  R289 KNotifications

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

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

To: albertvaka, #frameworks, apol
Cc: colomar, mck182, #frameworks

[Differential] [Commented On] D4663: Allow setting the timeout value.

[Albert Vaca Cintora]

albertvaka added a comment.


  I agree that just rising the timeout is suboptimal but I still like it better 
than a modal dialog... Actually, notifications were invented as a way to avoid 
harassing the user with modal dialogs when apps need attention.
  
  Apart from that, and even if we end up implementing this in a diferent way 
for KDE Connect, I don't think that KNotification as a framework should hide 
this feature from the developer. Regardless of the notification server 
implementation, the underlying protocol that KNotification exposes does allow 
defining a timeout.
  
  And, about SNIs... It's true that they have a title, text and actions like 
notifications. But they also happen to be represented in every major desktop 
(eg: Windows, MacOS, Gnome, Unity and Plasma) as an icon in the system tray 
that you have to right click to access a context menu with the actions (unlike 
notifications). Are you sure that's what Android does?
  
  Gnome implements something much more similar to the notification systems 
nowadays present in Android and iOS (and even Windows and MacOS, which are 
trying to deprecate their system tray icons), and they do that using the 
notifications cross-desktop standard. So I wouldn't criticize Gnome for doing 
their own thing here.

REPOSITORY
  R289 KNotifications

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

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

To: albertvaka, #frameworks, apol
Cc: mck182, #frameworks

[Differential] [Commented On] D4663: Allow setting the timeout value.

[Albert Vaca Cintora]

albertvaka added a comment.


  I don't think indicators are the future of notifications :/
  
  If you have a look at modern systems like Android (which had the luxury of 
designing a notification system without having to think about legacy), they 
have nothing like app indicators: they use persistent notifications instead. 
Gnome has also taken that way [1], so if we make appindicarors a core part of 
kdeconnect, we will never look native on Gnome... I'm trying to find a 
different solution.
  
  [1] https://wiki.gnome.org/Design/OS/MessageTray/Compatibility

REPOSITORY
  R289 KNotifications

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

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

To: albertvaka, #frameworks, apol
Cc: mck182, #frameworks

[Differential] [Commented On] D4663: Allow setting the timeout value.

[Albert Vaca Cintora]

albertvaka added a comment.


  I understand the potential of it being misused, even though I don't think we 
should treat developers as if they didn't know what they are doing. Also, if we 
want to have a maximum timeout, this is something that should be enforced on 
the server (ie: Plasma) instead of the client. There will be apps using 
libnotify or any other lib that can set their timeouts.
  
  The reason I added this is because in KDE Connect we show notifications when 
there is an incoming "pair request", and I wanted to make the notification stay 
there for as long as the request is valid. Since the notification is the only 
way to accept the pair request, I think it results in a bad user experience if 
the notification disappears before the user can act on it.
  
  There is an alternative solution to my use case: making the notification 
persistent and manually dismissing it when the request is no longer valid, but 
I've decided to not implement it this way because some notification servers 
don't support persistent notifications (eg: Ubuntu Unity).
  
  What do you think?

REPOSITORY
  R289 KNotifications

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

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

To: albertvaka, #frameworks, apol
Cc: mck182, #frameworks

[Differential] [Updated] D4663: Allow setting the timeout value.

[Albert Vaca Cintora]

albertvaka added a reviewer: apol.

REPOSITORY
  R289 KNotifications

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

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

To: albertvaka, #frameworks, apol
Cc: #frameworks

[Differential] [Request, 43 lines] D4663: Allow setting the timeout value.

[Albert Vaca Cintora]

albertvaka created this revision.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: Frameworks.

REPOSITORY
  R289 KNotifications

BRANCH
  master

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

AFFECTED FILES
  src/knotification.cpp
  src/knotification.h
  src/notifybypopup.cpp

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

To: albertvaka
Cc: #frameworks

[Differential] [Updated] D4663: Allow setting the timeout value.

[Albert Vaca Cintora]

albertvaka added a reviewer: Frameworks.

REPOSITORY
  R289 KNotifications

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

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

To: albertvaka, #frameworks
Cc: #frameworks

[Differential] [Closed] D4213: Mark non-persistent notifications as transient.

[Albert Vaca Cintora]

This revision was automatically updated to reflect the committed changes.
Closed by commit R289:579b75aac8e2: Mark non-persistent notifications as 
transient. (authored by albertvaka).

REPOSITORY
  R289 KNotifications

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D4213?vs=10371=10423

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

AFFECTED FILES
  src/notifybypopup.cpp

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

To: albertvaka, #frameworks, #plasma, apol
Cc: #frameworks

[Differential] [Updated] D4213: Mark non-persistent notifications as transient.

[Albert Vaca Cintora]

albertvaka added a reviewer: Frameworks.

REPOSITORY
  R289 KNotifications

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

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

To: albertvaka, #frameworks
Cc: #frameworks

[Differential] [Request, 4 lines] D4213: Mark non-persistent notifications as transient.

[Albert Vaca Cintora]

albertvaka created this revision.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: Frameworks.

REVISION SUMMARY
  On Gnome and other desktops using their extension of the notifications
  spec, notifications are persistent unless they set the transient hint.
  
  This tries to make notifications more consistent across desktops.

TEST PLAN
  Manual testing on Gnome.

REPOSITORY
  R289 KNotifications

BRANCH
  master

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

AFFECTED FILES
  src/notifybypopup.cpp

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

To: albertvaka
Cc: #frameworks

[Differential] [Closed] D4142: Support "default actions", as specified in [1].

[Albert Vaca Cintora]

This revision was automatically updated to reflect the committed changes.
Closed by commit R289:8b161b971fae: Support "default actions", as specified in 
[1]. (authored by albertvaka).

REPOSITORY
  R289 KNotifications

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D4142?vs=10186=10258

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

AFFECTED FILES
  src/knotification.cpp
  src/knotification.h
  src/notifybypopup.cpp

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

To: albertvaka, #frameworks, #vdg, mart
Cc: colomar, broulik, mart, #frameworks

[Differential] [Commented On] D4142: Support "default actions", as specified in [1].

[Albert Vaca Cintora]

albertvaka added a comment.


  In https://phabricator.kde.org/D4142#77834, @broulik wrote:
  
  > Care needs to be taken that existing notifications don't suddenly behave 
differently but that moving from actions() to defaultAction() is a conscious 
and explicit choice in the application that emits the notification.
  
  
  I've already made sure that existing apps behave as they currently do. Even 
more: apps that decide to move from setActions to setDefaultAction will still 
display exactly the same way in Plasma, with an action button (until/unless we 
change the Plasmoid to honor the default action, which we can discuss when I 
submit the patch for the Plasmoid).
  
  In https://phabricator.kde.org/D4142#77832, @mart wrote:
  
  > code wise the change makes sense..
  >  that said, this assumes we do an important behavior change in our ui, 
which i don't think is a good thing, as is disregards completely how our users 
are used to, in the name of "other platforms do things differently".
  
  
  No, there is no change in our UI at all. In Plasma and KDE apps we can decide 
to never use this feature if we think that the UX is better that way, but IMO 
this is a generic (not Plasma-specific) framework and it should provide access 
to the functionality anyway. This is a really common use case for notifications 
on most platforms, so if we want that some day this framework can be useful on, 
for example, Android or Gnome, this is something we have to provide. I think 
that it would be good to adopt it on Plasma and KDE apps, but this is a 
completely different discussion that we can have on the mailing list.
  
  And no, I don't think that our users will freak out if we start behaving like 
every other notification system.
  
  In https://phabricator.kde.org/D4142#77861, @colomar wrote:
  
  > Hm, that's not easy to decide. Whether one likes the "click to make go 
away" behavior or not is highly subjective. I, for one, find it very annoying 
on e.g. OS  when a notification covers something I want to see on the screen 
and there is no way to make it go away other than waiting. Others find it 
annoying to have to explicitly click a button to execute the default action.
  
  
  Same thing: the framework giving access to this feature doesn't mean your app 
has to use it. Even more: it doesn't even change the behaviour in Plasma! In 
Plasma the default action will still appear as a button unless we change the 
plasmoid. This will, however, make it possible to use this feature on desktops 
that do support default actions.

REPOSITORY
  R289 KNotifications

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

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

To: albertvaka, #frameworks, #vdg
Cc: colomar, broulik, mart, #frameworks

[Differential] [Request, 57 lines] D4142: Support "default actions", as specified in [1].

[Albert Vaca Cintora]

albertvaka created this revision.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: Frameworks.

REVISION SUMMARY
  Default actions are the actions triggered when the notification is simply
  clicked, without adding any "action buttons". Until now, with
  KNotifications we could only make notifications that go away when clicked,
  which might not be what the user expects (see notifications on smartphones
  or other desktop environments).
  
  Since KNotifications uses an int->name mapping (instead of string->name),
  we need to manually convert between "default" and "0". Interestingly
  enough, there was already code to handle the action "0" and emit
  activated(), which is documented as "Emitted only when the default
  activation has occurred" but in practice was never actually used.
  
  [1] 
https://people.gnome.org/~mccann/docs/notification-spec/notification-spec-latest.html

TEST PLAN
  Tests pass.

REPOSITORY
  R289 KNotifications

BRANCH
  master

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

AFFECTED FILES
  src/knotification.cpp
  src/knotification.h
  src/knotificationmanager.cpp
  src/notifybypopup.cpp

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

To: albertvaka
Cc: #frameworks

[Differential] [Commented On] D2545: Cleanup KSharedUiServerProxy before qApp exists

[albertvaka (Albert Vaca Cintora)]

albertvaka added a comment.


  Since there is no fix on Qt, should we merge this?

BRANCH
  master

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

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

To: kfunk, vonreth, dfaure
Cc: albertvaka, mutlaqja, arrowdodger, #frameworks

[Differential] [Commented On] D2546: Cleanup DBus-related resources before qApp exits

[albertvaka (Albert Vaca Cintora)]

albertvaka added a comment.


  Since there is no fix on Qt, should we merge this?

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

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

To: kfunk, vonreth, dfaure
Cc: albertvaka, #frameworks

Re: Review Request 127169: By default, make KDE_INSTALL_USE_QT_SYS_PATHS share the same directory scheme as Qt if they share the prefix

[Albert Vaca Cintora]

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


Ship it!




I don't see a reason not to change it.

It makes a lot of sense to install things in the same places as Qt, especially 
when Qt has to find the stuff we install.

- Albert Vaca Cintora


On feb. 24, 2016, 9:09 a.m., Aleix Pol Gonzalez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127169/
> ---
> 
> (Updated feb. 24, 2016, 9:09 a.m.)
> 
> 
> Review request for Extra Cmake Modules, KDE Frameworks and Albert Vaca 
> Cintora.
> 
> 
> Repository: extra-cmake-modules
> 
> 
> Description
> ---
> 
> Make Qt and ECM-based projects use the same directory sctructure (i.e. where 
> plugins are, libs, etc.) by default. Otherwise it creates a tiny mess that 
> might be controlled but usually won't.
> 
> In the end, otherwise, people need to keep adapting their systems with 
> environment variables anyway. All distros end up setting always this setting 
> as ON, as well as brave developers who don't have separate prefixes for Qt 
> and KDE.
> 
> 
> Diffs
> -
> 
>   kde-modules/KDEInstallDirs.cmake ebd48fa 
> 
> Diff: https://git.reviewboard.kde.org/r/127169/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 122206: [kio] Make tests optional

[Albert Vaca Cintora]

 On March 16, 2015, 8:37 p.m., Albert Vaca Cintora wrote:
  I know this is merged already but this patch is being applied to every KDE 
  package and I want to keep the discussion in a single place.
  
  We already have a toggle option in CMake that is BUILD_TESTING. If Gentoo 
  wants to not build the tests (I'm not judging if they should, let them be 
  free to do it), they can just set BUILD_TESTING to OFF. I understand that 
  CMake will still try to find Qt5Test and fail, but here is where I think we 
  got it wrong:
  
  This patch does the following:
  if (Qt5Test is not found) 
  BUILD_TESTING = OFF
  
  What I think this patch should be doing is this:
  if (BUILD_TESTING == OFF) 
  Don't look for Qt5Test
  
  Did I miss something or this seems more reasonable to you guys as well?
 
 Michael Palimaka wrote:
 One of the original versions of these test patches looked something like:
 
 if (BUILD_TESTING)
 add_subdirectory(autotests)
 endif ()
 
 with `find_package(Qt5Test)` inside the autotests directory. While this 
 is used a bit in frameworks, it was rejected from a lot of plasma packages 
 because it relies on a magic variable (although it is defined by ECM).
 
 As a result there's a whole range of approaches across 
 frameworks/plasma/apps all doing the same thing. It would be nice if we could 
 agree on something and be consistent.

Is that the reason why we are doing it this way? :/ I don't think this is a 
magic variable at all, and if you want it to be even less magic you can set it 
in advance in a line before the if.


- Albert


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


On Feb. 6, 2015, 4:14 p.m., Andreas Sturmlechner wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/122206/
 ---
 
 (Updated Feb. 6, 2015, 4:14 p.m.)
 
 
 Review request for KDE Frameworks.
 
 
 Repository: kio
 
 
 Description
 ---
 
 [kio] Make tests optional
 This is a small patch to CMakeLists.txt to only depend on Qt5Test if 
 BUILD_TESTING.
 
 
 Diffs
 -
 
   CMakeLists.txt c1ed03f6cac648517828aec60e896baf9fbcfd9d 
 
 Diff: https://git.reviewboard.kde.org/r/122206/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Andreas Sturmlechner
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 122206: [kio] Make tests optional

[Albert Vaca Cintora]

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


I know this is merged already but this patch is being applied to every KDE 
package and I want to keep the discussion in a single place.

We already have a toggle option in CMake that is BUILD_TESTING. If Gentoo 
wants to not build the tests (I'm not judging if they should, let them be free 
to do it), they can just set BUILD_TESTING to OFF. I understand that CMake will 
still try to find Qt5Test and fail, but here is where I think we got it wrong:

This patch does the following:
if (Qt5Test is not found) 
BUILD_TESTING = OFF

What I think this patch should be doing is this:
if (BUILD_TESTING == OFF) 
Don't look for Qt5Test

Did I miss something or this seems more reasonable to you guys as well?

- Albert Vaca Cintora


On Feb. 6, 2015, 4:14 p.m., Andreas Sturmlechner wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/122206/
 ---
 
 (Updated Feb. 6, 2015, 4:14 p.m.)
 
 
 Review request for KDE Frameworks.
 
 
 Repository: kio
 
 
 Description
 ---
 
 [kio] Make tests optional
 This is a small patch to CMakeLists.txt to only depend on Qt5Test if 
 BUILD_TESTING.
 
 
 Diffs
 -
 
   CMakeLists.txt c1ed03f6cac648517828aec60e896baf9fbcfd9d 
 
 Diff: https://git.reviewboard.kde.org/r/122206/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Andreas Sturmlechner
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 122913: Added an event() version that takes no icon and will use a default one

[Albert Vaca Cintora]

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

(Updated March 14, 2015, 4:58 a.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks and Martin Klapetek.


Changes
---

Submitted with commit b368fb958df0ce4cc4128029f5649640f46d1559 by Albert Vaca 
to branch master.


Repository: knotifications


Description
---

Added an event() version that takes no icon and will use a default one 
depending on the notification type


Diffs
-

  src/knotification.h f2dcd74e26a4feefe53dc0e536b0178d5ce287e1 
  src/knotification.cpp 293de09bae8d16b77df81ee2fe447c3246a476b5 

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


Testing
---


Thanks,

Albert Vaca Cintora

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 122913: Added an event() version that takes no icon and will use a default one

[Albert Vaca Cintora]

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

(Updated mar. 14, 2015, 5:57 a.m.)


Review request for KDE Frameworks and Martin Klapetek.


Repository: knotifications


Description
---

Added an event() version that takes no icon and will use a default one 
depending on the notification type


Diffs (updated)
-

  src/knotification.h f2dcd74e26a4feefe53dc0e536b0178d5ce287e1 
  src/knotification.cpp 293de09bae8d16b77df81ee2fe447c3246a476b5 

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


Testing
---


Thanks,

Albert Vaca Cintora

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 122864: Added event() version that takes StandardEvent eventId and QString iconName

[Albert Vaca Cintora]

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

(Updated March 14, 2015, 4:58 a.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks and Aleix Pol Gonzalez.


Changes
---

Submitted with commit dedf55c937f95259121c4985f07345e06dd5b47b by Albert Vaca 
to branch master.


Repository: knotifications


Description
---

This allows to use icon names instead of QPixmaps when using StandardEvents
instead of app-defined events.


Diffs
-

  src/knotification.cpp 293de09bae8d16b77df81ee2fe447c3246a476b5 
  src/knotification.h f2dcd74e26a4feefe53dc0e536b0178d5ce287e1 

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


Testing
---


Thanks,

Albert Vaca Cintora

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 122864: Added event() version that takes StandardEvent eventId and QString iconName

[Albert Vaca Cintora]

 On mar. 9, 2015, 11:20 a.m., Martin Klapetek wrote:
  Ship It!
 
 Martin Klapetek wrote:
 Actually wait, StandardEvents should have standard icons, why would you 
 need to change them?
 
 Albert Vaca Cintora wrote:
 They don't have a standard, default icon: you have to specify it and as 
 of now the only way you have for it is with a QPixmap.
 
 Martin Klapetek wrote:
 Indeed, you are correct. Ship this then, it makes sense to have an 
 alternative for the QPixmap only.
 
 Another overload should be added that actually uses the default icons 
 automatically though. Would you be up for that?

Sure: https://git.reviewboard.kde.org/r/122913/


- Albert


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


On mar. 9, 2015, 6:02 a.m., Albert Vaca Cintora wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/122864/
 ---
 
 (Updated mar. 9, 2015, 6:02 a.m.)
 
 
 Review request for KDE Frameworks and Aleix Pol Gonzalez.
 
 
 Repository: knotifications
 
 
 Description
 ---
 
 This allows to use icon names instead of QPixmaps when using StandardEvents
 instead of app-defined events.
 
 
 Diffs
 -
 
   src/knotification.cpp 293de09bae8d16b77df81ee2fe447c3246a476b5 
   src/knotification.h f2dcd74e26a4feefe53dc0e536b0178d5ce287e1 
 
 Diff: https://git.reviewboard.kde.org/r/122864/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Albert Vaca Cintora
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 122900: KPluginSelector to provide initialization arguments for the configuration modules

[Albert Vaca Cintora]

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

Ship it!


It even works! :D


src/kpluginselector.h
https://git.reviewboard.kde.org/r/122900/#comment53110

@since 5.9



src/kpluginselector.h
https://git.reviewboard.kde.org/r/122900/#comment53111

@since 5.9


- Albert Vaca Cintora


On mar. 11, 2015, 1:53 a.m., Aleix Pol Gonzalez wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/122900/
 ---
 
 (Updated mar. 11, 2015, 1:53 a.m.)
 
 
 Review request for kdeconnect and KDE Frameworks.
 
 
 Repository: kcmutils
 
 
 Description
 ---
 
 KCMs make it possible to receive a list of arguments we're not passing yet. 
 This patch makes it possible to specify these arguments by specifying a 
 static list of arguments for all the plugins in the selector.
 
 
 Diffs
 -
 
   src/kpluginselector.h 0125991 
   src/kpluginselector.cpp 98ab59e 
   src/kpluginselector_p.h d80cd2e 
 
 Diff: https://git.reviewboard.kde.org/r/122900/diff/
 
 
 Testing
 ---
 
 Tested over at kdeconnect, where we need this.
 
 
 Thanks,
 
 Aleix Pol Gonzalez
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Review Request 122913: Added an event() version that takes no icon and will use a default one

[Albert Vaca Cintora]

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

Review request for KDE Frameworks and Martin Klapetek.


Repository: knotifications


Description
---

Added an event() version that takes no icon and will use a default one 
depending on the notification type


Diffs
-

  src/knotification.h f2dcd74e26a4feefe53dc0e536b0178d5ce287e1 
  src/knotification.cpp 293de09bae8d16b77df81ee2fe447c3246a476b5 

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


Testing
---


Thanks,

Albert Vaca Cintora

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 122913: Added an event() version that takes no icon and will use a default one

[Albert Vaca Cintora]

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

(Updated mar. 12, 2015, 4:37 a.m.)


Review request for KDE Frameworks and Martin Klapetek.


Changes
---

Had to remove a default empty QPixmap parameter from a different event()
header, that would make a call to one of them ambiguous. This means that
when recompiling an app that used event() with StandardEvent and didn't
specify an icon, it will now pick the version that uses standard icons
instead of the one that shows no icon at all. (Which seems ok to me).


Repository: knotifications


Description
---

Added an event() version that takes no icon and will use a default one 
depending on the notification type


Diffs (updated)
-

  src/knotification.h f2dcd74e26a4feefe53dc0e536b0178d5ce287e1 
  src/knotification.cpp 293de09bae8d16b77df81ee2fe447c3246a476b5 

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


Testing
---


Thanks,

Albert Vaca Cintora

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 122864: Added event() version that takes StandardEvent eventId and QString iconName

[Albert Vaca Cintora]

 On mar. 9, 2015, 11:20 a.m., Martin Klapetek wrote:
  Ship It!
 
 Martin Klapetek wrote:
 Actually wait, StandardEvents should have standard icons, why would you 
 need to change them?

They don't have a standard, default icon: you have to specify it and as of now 
the only way you have for it is with a QPixmap.


- Albert


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


On mar. 9, 2015, 6:02 a.m., Albert Vaca Cintora wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/122864/
 ---
 
 (Updated mar. 9, 2015, 6:02 a.m.)
 
 
 Review request for KDE Frameworks and Aleix Pol Gonzalez.
 
 
 Repository: knotifications
 
 
 Description
 ---
 
 This allows to use icon names instead of QPixmaps when using StandardEvents
 instead of app-defined events.
 
 
 Diffs
 -
 
   src/knotification.cpp 293de09bae8d16b77df81ee2fe447c3246a476b5 
   src/knotification.h f2dcd74e26a4feefe53dc0e536b0178d5ce287e1 
 
 Diff: https://git.reviewboard.kde.org/r/122864/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Albert Vaca Cintora
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Review Request 122864: Added event() version that takes StandardEvent eventId and QString iconName

[Albert Vaca Cintora]

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

Review request for KDE Frameworks and Aleix Pol Gonzalez.


Repository: knotifications


Description
---

This allows to use icon names instead of QPixmaps when using StandardEvents
instead of app-defined events.


Diffs
-

  src/knotification.cpp 293de09bae8d16b77df81ee2fe447c3246a476b5 
  src/knotification.h f2dcd74e26a4feefe53dc0e536b0178d5ce287e1 

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


Testing
---


Thanks,

Albert Vaca Cintora

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 112177: Split URL drop functionality from KLineEdit

[Albert Vaca Cintora]

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

(Updated Aug. 20, 2013, 8:46 p.m.)


Review request for KDE Frameworks.


Description
---

The replace content on URL drop functionality that was present in KLineEdit has 
been moved to a separate class, that can be installed as an event filter in any 
QLineEdit (KLineEdit, QComboBox, KUrlRequester...). 

KLineEdit now uses this new class when the property urlDropsEnabled is 
enabled, but this property has been marked as deprecated in favour of the new 
alternative.


Diffs
-

  tier1/kwidgetsaddons/src/lineediturldropeventfilter.cpp PRE-CREATION 
  tier1/kwidgetsaddons/src/lineediturldropeventfilter.h PRE-CREATION 
  tier1/kwidgetsaddons/src/CMakeLists.txt c17c648 
  staging/kcompletion/src/klineedit.cpp 213b196 
  staging/kcompletion/src/klineedit.h e9f3332 
  staging/kcompletion/src/kcombobox.h 0d4e912 

Diff: http://git.reviewboard.kde.org/r/112177/diff/


Testing
---

Manual testing + tests passed


Thanks,

Albert Vaca Cintora

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 111184: Url drop functionality in KUrlRequester does not depend on KLineEdit (since we have to port it to QLineEdit)

[Albert Vaca Cintora]

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

(Updated July 16, 2013, 10:03 p.m.)


Status
--

This change has been discarded.


Review request for KDE Frameworks.


Description
---

KLineEdit and KComboBox had the property enableUrlDrops that was added to use 
it in KUrlRequester. Since KUrlRequester is part of KIO, it can not use 
Kde4Support and should be ported to use QLineEdit and QComboBox. As I said on 
my email to the frameworks list, it was easier to implement this directly in 
KUrlRequester than patching Qt, even though it was originally planned as a Qt 
task. It was my intention to also remove that property from KLineEdit and 
KComboBox, but finally I didn't include it in the patch (see Ervin's comment in 
the mailing list).

In the patch I have also added a TODO because I think it would be good to 
detect any url without needing the mimetype to be set, but I have not done it 
yet because it should be discussed first.

(Reviewed by aacid and apol)


Diffs
-

  KDE5PORTING.html ba67bdc 
  kdeui/widgets/kcombobox.h ccb019d 
  kdeui/widgets/klineedit.h 7ac22f6 
  kio/kfile/kurlrequester.cpp fa6b234 

Diff: http://git.reviewboard.kde.org/r/84/diff/


Testing
---


Thanks,

Albert Vaca Cintora

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 111184: Url drop functionality in KUrlRequester does not depend on KLineEdit (since we have to port it to QLineEdit)

[Albert Vaca Cintora]

 On June 25, 2013, 6:46 a.m., Kevin Ottens wrote:
  kio/kfile/kurlrequester.cpp, line 483
  http://git.reviewboard.kde.org/r/84/diff/2/?file=165624#file165624line483
 
  Why don't you use KUrlMimeData like the original code in KLineEdit? 
  It'll be more robust AFAICT.
 
 Albert Vaca Cintora wrote:
 I'm using hasUrls() instead because KUrlMimeData returns a list of urls, 
 which is more expensive than what Qt does (a string comparison). 
 
 Also, if our way of checking URLs is better than the way Qt uses, we 
 should contribute it to Qt instead of avoid using their methods (we are going 
 to rely a lot more on Qt since KF5, so we have to trust what Qt does).
 
 Kevin Ottens wrote:
 Well, KUrlMimeData has some specific handling for KIO, which is a big 
 no-no for Qt. That's more what I had in mind... we don't need the metadata 
 part in that context though, so it's likely a moot point.
 
 That said we look for the application/x-kde4-urilist mimetype too (again 
 something we can't really push in Qt). David, any reason why we have this 
 extra mimetype? Doesn't really makes sense to me.
 
 David Faure wrote:
 KUrlMimeData has some specific handling for KIO, and not just metadata, 
 but also shipping two lists of URLs: KIO urls and most-local urls. For 
 instance desktop:/foo and file://home/dfaure/Desktop/foo, the first one being 
 aimed at KIO-enabled apps and the second one at non-KIO-enabled apps.
 
 The code here simply checks for there are urls, and lets QLineEdit do 
 the extraction.
 This means that the most-local urls are going to be pasted, not the KIO 
 ones.
 But that's not really specific to kurlrequester nor the 
 clear-before-insert feature... (e.g. konqueror's location bar would have the 
 same issue)
 
 WAIT . who determined that only KUrlRequester used the url drop 
 feature of KLineEdit?
 That feature is ON by default, so looking for calls to 
 setUrlDropsEnabled(true) is definitely wrong.
 Konqueror's location bar definitely uses that feature too. I suspect the 
 location bar in dolphin and the one in kfiledialog do as well, at least.
 
 It seems to me that putting the code into KUrlRequester is just wrong. It 
 should be an event filter that can be plugged onto any QLineEdit. And then it 
 can take care of both: clear before insert, and using KUrlMimeData to do the 
 URL extraction.
 
 Kevin Ottens wrote:
 Hmmm... You're right, as I mentionned earlier that was my only concern 
 that url drops were enabled by default... But somehow I assumed that most of 
 our URL bars were KUrlRequester. Dunno what happened in my brain that day as 
 obviously it's not the case. They're either KComboBox or KLineEdit.
 
 OK... so if we want to make the url drops feature more reusable we should 
 make that feature an event filter in its own class as I initially proposed.
 
 But (and that's a strong but), I now realize that we never pushed toward 
 KUrlRequester to use QLineEdit or QComboBox. It can keep using KLineEdit or 
 KComboBox just fine. They're not getting deprecated, they'll be in 
 KCompletion (and the future KioWidgets will be able to use KCompletion just 
 fine IMO).
 
 It makes me think this patch should in fact be dropped.

I see your point and agree with you. However I don't think that every other 
class using this feature can keep using KLineEdit as before (some will need to 
be ported to QLineEdit even if I was wrong and this is not the case of 
KUrlRequester) and I don't think this feature is generic enough to send a patch 
to Qt. I could then move it to a separate class (usable from wherever we need) 
as Ervin suggested initially, but... do you feel it necessary or can we just 
drop this feature from the apps we migrate to QLineEdit? 

Since you have a more comprehensive perspective of the project as I have, if 
you think we should keep it I will write the patch to move it to an independent 
class.


- Albert


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/84/#review35019
---


On June 25, 2013, 10:46 a.m., Albert Vaca Cintora wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/84/
 ---
 
 (Updated June 25, 2013, 10:46 a.m.)
 
 
 Review request for KDE Frameworks.
 
 
 Description
 ---
 
 KLineEdit and KComboBox had the property enableUrlDrops that was added to use 
 it in KUrlRequester. Since KUrlRequester is part of KIO, it can not use 
 Kde4Support and should be ported to use QLineEdit and QComboBox. As I said on 
 my email to the frameworks list, it was easier to implement this directly in 
 KUrlRequester than patching Qt, even though

Re: Review Request 111184: Url drop functionality in KUrlRequester does not depend on KLineEdit (since we have to port it to QLineEdit)

[Albert Vaca Cintora]

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

(Updated June 25, 2013, 10:46 a.m.)


Review request for KDE Frameworks.


Description
---

KLineEdit and KComboBox had the property enableUrlDrops that was added to use 
it in KUrlRequester. Since KUrlRequester is part of KIO, it can not use 
Kde4Support and should be ported to use QLineEdit and QComboBox. As I said on 
my email to the frameworks list, it was easier to implement this directly in 
KUrlRequester than patching Qt, even though it was originally planned as a Qt 
task. It was my intention to also remove that property from KLineEdit and 
KComboBox, but finally I didn't include it in the patch (see Ervin's comment in 
the mailing list).

In the patch I have also added a TODO because I think it would be good to 
detect any url without needing the mimetype to be set, but I have not done it 
yet because it should be discussed first.

(Reviewed by aacid and apol)


Diffs (updated)
-

  KDE5PORTING.html ba67bdc 
  kdeui/widgets/kcombobox.h ccb019d 
  kdeui/widgets/klineedit.h 7ac22f6 
  kio/kfile/kurlrequester.cpp fa6b234 

Diff: http://git.reviewboard.kde.org/r/84/diff/


Testing
---


Thanks,

Albert Vaca Cintora

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Review Request 111184: Moved url drop functionality from KLineEdit to KUrlChooser

[Albert Vaca Cintora]

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

Review request for KDE Frameworks.


Description
---

KLineEdit and KComboBox had the property enableUrlDrops that was added to use 
it in KUrlRequester. Since KUrlRequester is part of KIO, it can not use 
Kde4Support and should be ported to use QLineEdit and QComboBox. As I said on 
my email to the frameworks list, it was easier to implement this directly in 
KUrlRequester than patching Qt, even though it was originally planned as a Qt 
task. It was my intention to also remove that property from KLineEdit and 
KComboBox, but finally I didn't include it in the patch (see Ervin's comment in 
the mailing list).

In the patch I have also added a TODO because I think it would be good to 
detect any url without needing the mimetype to be set, but I have not done it 
yet because it should be discussed first.

(Reviewed by aacid and apol)


Diffs
-

  kio/kfile/kurlrequester.cpp fa6b234 

Diff: http://git.reviewboard.kde.org/r/84/diff/


Testing
---


Thanks,

Albert Vaca Cintora

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 111184: Url drop functionality in KUrlRequester does not depend on KLineEdit (since we have to port it to QLineEdit)

[Albert Vaca Cintora]

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

(Updated June 24, 2013, 10:15 p.m.)


Review request for KDE Frameworks.


Summary (updated)
-

Url drop functionality in KUrlRequester does not depend on KLineEdit (since we 
have to port it to QLineEdit)


Description
---

KLineEdit and KComboBox had the property enableUrlDrops that was added to use 
it in KUrlRequester. Since KUrlRequester is part of KIO, it can not use 
Kde4Support and should be ported to use QLineEdit and QComboBox. As I said on 
my email to the frameworks list, it was easier to implement this directly in 
KUrlRequester than patching Qt, even though it was originally planned as a Qt 
task. It was my intention to also remove that property from KLineEdit and 
KComboBox, but finally I didn't include it in the patch (see Ervin's comment in 
the mailing list).

In the patch I have also added a TODO because I think it would be good to 
detect any url without needing the mimetype to be set, but I have not done it 
yet because it should be discussed first.

(Reviewed by aacid and apol)


Diffs
-

  kio/kfile/kurlrequester.cpp fa6b234 

Diff: http://git.reviewboard.kde.org/r/84/diff/


Testing
---


Thanks,

Albert Vaca Cintora

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel