Test failures with krunner 5.39.0

2017-11-01 Thread Hartmut Goebel
Hallo,

I'm packaging krunner 5.39.0 for GNU Guix [1]. GNU Guix builds all
software in an isolated environment (a container), which includes only
the packages and tools specified explicitly.

When running the tests, dbusrunnertest faills, with this notable warning
and the error show below:

   DBusRunnerTest::testMatch() org.kde.krunner: Invalid entry: "DBus
runner test"

Any idea what may be missing, what needs to be set up (env-var,
directory, package, paths)? Any idea hos to debug this further?

All required packages and all optional/feature packages (except of QCH)
are installed. I tried running the tests with both dbus-launch and
without. The external program "testremoterunner" gets started. strace -e
trace=file" shows nothing relevant (as far as I can tell).

* Start testing of DBusRunnerTest *
Config: Using QtTest library 5.9.2, Qt 5.9.2 (x86_64-little_endian-lp64
shared (dynamic) release build; by GCC 5.4.0)
PASS   : DBusRunnerTest::initTestCase()
QWARN  : DBusRunnerTest::testMatch() inotify_add_watch("/etc/mtab")
failed: "No such file or directory"
QWARN  : DBusRunnerTest::testMatch() inotify_add_watch("/etc/fstab")
failed: "No such file or directory"
QWARN  : DBusRunnerTest::testMatch() org.kde.krunner: Invalid entry:
"DBus runner test"
FAIL!  : DBusRunnerTest::testMatch() 'spy.wait()' returned FALSE. ()
   Loc:
[/tmp/guix-build-krunner-5.39.0.drv-0/krunner-5.39.0/autotests/dbusrunnertest.cpp(82)]
PASS   : DBusRunnerTest::cleanupTestCase()
Totals: 2 passed, 1 failed, 0 skipped, 0 blacklisted, 4931ms
* Finished testing of DBusRunnerTest *

-- 
Regards
Hartmut Goebel

| Hartmut Goebel  | h.goe...@crazy-compilers.com   |
| www.crazy-compilers.com | compilers which you thought are impossible |




D8098: Strip down and re-write the tags KIO slave.

2017-11-01 Thread David Faure
dfaure added inline comments.

INLINE COMMENTS

> kio_tags.cpp:64
> +QString tag;
> +tag = url.path().section(QLatin1Char('?'), 0, 0);
> +while (tag.startsWith(QDir::separator()))

- merge with previous line

- can path() really contain '?' ? I thought that wasn't possible (? delimits 
the query).

> kio_tags.cpp:119
> +Q_UNUSED(permissions);
> +Q_UNUSED(flags);
>  

missing KIO::Overwrite support? Can the dest already exist?

> kio_tags.cpp:156
>  
> -case FileUrl:
> -ForwardingSlaveBase::get(QUrl::fromLocalFile(fileUrl));
> -return;
> +if ((result.urlType == TagFileUrl) || (result.urlType == LocalUrl)) {
> +ForwardingSlaveBase::get(QUrl::fromLocalFile(result.filePath));

And otherwise? what happens if it's neither TagFileUrl nor LocalUrl? should 
error(not supported) be called? The job would never finish otherwise.

> kio_tags.cpp:163
>  {
> -Q_UNUSED(permissions);
>  Q_UNUSED(flags);
>  

same question, can dest already exist?
(if so, then the right thing to do is error(), unless `flags & Overwrite`)

> kio_tags.cpp:203
> +for (const QString& tag : md.tags()) {
> +if (tag == srcResult.tag || 
> (tag.startsWith(QString(srcResult.tag + QLatin1Char('/') {
> +QString newTag = tag;

the QString() isn't needed  (repeats)

> kio_tags.cpp:262
> +} else {
> +ForwardingSlaveBase::mimetype(QUrl::fromLocalFile(result.filePath));
>  }

missing return, to avoid emitting finished() twice
(like you did after the other calls to ForwardingSlaveBase)

> kio_tags.cpp:292
> +TagListJob* tagJob = new TagListJob();
> +tagJob->exec();
>  

can this fail?

> kio_tags.cpp:331
> +// Extract any file path and determine tag from url
> +if (url.scheme() == QLatin1String("file")) {
> +result.urlType = LocalUrl;

That's better written as if (url.isLocalFile())

> kio_tags.cpp:337
> +bool validTag = relaxValidation;
> +if (url.path().contains(QLatin1Char('?')) || chopLastSection) {
> +QUrl choppedUrl = url;

same question as before

> kio_tags.cpp:356
> +QString query;
> +const QStringList tagSections = 
> result.tag.split(QLatin1Char('/'));
> +for (int i = 0; i < tagSections.size(); ++i) {

could this whole splitting be optimized by saying something like this?

  result.tag.prepend("tag:");
  result.tag.replace('/', " AND tag:");

It's just that split is slow (allocates a temporary container etc.).
If this is called often, and the above isn't good enough, then something like 
what I did in kcontacts, commit 
https://phabricator.kde.org/R174:5de361ff80ce0a015b62a3a76fe676355d8d3a52, can 
be used.

But maybe split is fine here, I guess it's likely not time critical.

> kio_tags.cpp:393
> +result.pathUDSResults << createUDSEntryForTag(QStringLiteral("."), 
> result.tag);
> +int index = result.tag.count(QLatin1Char('/')) + !result.tag.isEmpty();
> +QStringList tagPaths;

This conversion from bool to int sounds ... fragile.
I would do

  + (result.tag.isEmpty() ? 0 : 1)

> kio_tags.cpp:397
> +for (const QString& tag : tags) {
> +QString tagSection = tag.section(QLatin1Char('/'), index, index, 
> QString::SectionSkipEmpty);
> +if (result.tag.isEmpty() || (tag.startsWith(result.tag, 
> Qt::CaseInsensitive) && !result.tag.isEmpty())) {

`tagSection` isn't used by the if condition itself, so it could move into the 
if().

> kio_tags.cpp:398
> +QString tagSection = tag.section(QLatin1Char('/'), index, index, 
> QString::SectionSkipEmpty);
> +if (result.tag.isEmpty() || (tag.startsWith(result.tag, 
> Qt::CaseInsensitive) && !result.tag.isEmpty())) {
> +if (!tagPaths.contains(tagSection, Qt::CaseInsensitive) && 
> !tagSection.isEmpty()) {

The `&& !result.tag.isEmpty()` is useless.
If we're evaluating that part of the condition (the part after the "||"), then 
result.tag is NOT empty, by definition.

> kio_tags.cpp:422
> +KIO::UDSEntry uds;
> +if (KIO::StatJob* job = KIO::stat(match, KIO::HideProgressInfo)) {
> +// we do not want to wait for the event loop to delete the job

KIO::stat never returns nullptr, so the if() isn't useful.

REPOSITORY
  R293 Baloo

BRANCH
  master-nestedTags (branched from master)

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

To: smithjd, #frameworks, vhanda, #dolphin, ngraham
Cc: dfaure, nicolasfella, ngraham


D8098: Strip down and re-write the tags KIO slave.

2017-11-01 Thread David Faure
dfaure requested changes to this revision.
This revision now requires changes to proceed.

REPOSITORY
  R293 Baloo

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

To: smithjd, #frameworks, vhanda, #dolphin, ngraham, dfaure
Cc: dfaure, nicolasfella, ngraham


D8084: KAutoSaveFile breaks if source file name contains a space!

2017-11-01 Thread David Faure
dfaure added a comment.


  Thanks for the fix and autotest !

INLINE COMMENTS

> kautosavefiletest.cpp:83
>  {
> -// TODO
> +QUrl normalFile = QUrl::fromLocalFile(QStringLiteral("/tmp/test 
> directory/tîst me.txt"));
> +

Should use QDir::tempPath() for portability.

REPOSITORY
  R244 KCoreAddons

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

To: mardelle, #frameworks, shaforostoff
Cc: dfaure, ngraham, cfeck, ltoscano


D8544: KTextEditor : avoiding QML crashes

2017-11-01 Thread René J . V . Bertin
rjvbb set the repository for this revision to R39 KTextEditor.

REPOSITORY
  R39 KTextEditor

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

To: rjvbb, #frameworks
Cc: cullmann, kde-frameworks-devel, kevinapavew, demsking, head7, kfunk, sars, 
dhaumann


D8544: KTextEditor : avoiding QML crashes

2017-11-01 Thread René J . V . Bertin
rjvbb updated this revision to Diff 21724.
rjvbb added a comment.


  This simpler version also seems to do the trick for me (read: I haven't been 
able to get "it" to crash).

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8544?vs=21519=21724

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

AFFECTED FILES
  src/utils/kateglobal.cpp

To: rjvbb, #frameworks
Cc: cullmann, kde-frameworks-devel, kevinapavew, demsking, head7, kfunk, sars, 
dhaumann


D8566: Add API to retrieve the screen id for a screen name

2017-11-01 Thread Andras Mantia
amantia added a dependent revision: D8493: Make Folder View screen aware.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: amantia, #plasma, ervin, dvratil, mlaurent, hein, davidedmundson
Cc: broulik, plasma-devel, dhaumann, apol, #frameworks, ZrenBot, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart


D8332: Added baloo urls into places model

2017-11-01 Thread Nathaniel Graham
ngraham added a comment.


  @dvratil and @ervin, any more concerns?

REPOSITORY
  R241 KIO

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

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, 
ervin
Cc: ervin, usta, mlaurent, dvratil, ngraham, #frameworks


D8434: Created 'remote' section

2017-11-01 Thread Nathaniel Graham
ngraham accepted this revision.
ngraham added a comment.


  Lovely. Looks good to me, too!

REPOSITORY
  R241 KIO

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

To: renatoo, ngraham, #frameworks, #dolphin, mwolff
Cc: elvisangelaccio, mwolff, mlaurent, #frameworks


D8348: Add a section for removable devices

2017-11-01 Thread Renato Oliveira Filho
renatoo added a dependent revision: D8434: Created 'remote' section.

REPOSITORY
  R241 KIO

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

To: renatoo, #dolphin, #frameworks, #vdg, ervin
Cc: mlaurent, anthonyfieroni, ngraham, #frameworks


D8434: Created 'remote' section

2017-11-01 Thread Renato Oliveira Filho
renatoo edited the summary of this revision.
renatoo added dependencies: D8332: Added baloo urls into places model, D8348: 
Add a section for removable devices.

REPOSITORY
  R241 KIO

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

To: renatoo, ngraham, #frameworks, #dolphin, mwolff
Cc: elvisangelaccio, mwolff, mlaurent, #frameworks


D8332: Added baloo urls into places model

2017-11-01 Thread Renato Oliveira Filho
renatoo added a dependent revision: D8434: Created 'remote' section.

REPOSITORY
  R241 KIO

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

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, 
ervin
Cc: ervin, usta, mlaurent, dvratil, ngraham, #frameworks


D8434: Created 'remote' section

2017-11-01 Thread Nathaniel Graham
ngraham added a comment.


  Can you add "Depends on D" for each other patch that this requires?

REPOSITORY
  R241 KIO

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

To: renatoo, ngraham, #frameworks, #dolphin, mwolff
Cc: elvisangelaccio, mwolff, mlaurent, #frameworks


D8434: Created 'remote' section

2017-11-01 Thread Milian Wolff
mwolff accepted this revision.
mwolff added a comment.
This revision is now accepted and ready to land.


  lgtm, but please wait a bit, maybe someone else wants to chime in?

REPOSITORY
  R241 KIO

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

To: renatoo, ngraham, #frameworks, #dolphin, mwolff
Cc: elvisangelaccio, mwolff, mlaurent, #frameworks


D8592: Update default colors to match new colors in D7424

2017-11-01 Thread Nathaniel Graham
ngraham created this revision.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: Frameworks.

REVISION SUMMARY
  If https://phabricator.kde.org/D7424 goes in, we'll have to update the 
default colors in KConfigWidgets. This does that.
  
  Depends on https://phabricator.kde.org/D7424

TEST PLAN
  Compiled and deployed in KDE Neon, then ran a bunch of programs. Could not 
see any differences.

REPOSITORY
  R265 KConfigWidgets

BRANCH
  master

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

AFFECTED FILES
  src/kcolorscheme.cpp

To: ngraham
Cc: #frameworks


D8592: Update default colors to match new colors in D7424

2017-11-01 Thread Nathaniel Graham
ngraham added reviewers: Frameworks, broulik, VDG.

REPOSITORY
  R265 KConfigWidgets

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

To: ngraham, #frameworks, broulik, #vdg
Cc: #frameworks


D8434: Created 'remote' section

2017-11-01 Thread Renato Oliveira Filho
renatoo updated this revision to Diff 21697.
renatoo added a comment.


  Added more test cases
  Renamed enum to match group name

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8434?vs=21694=21697

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

AFFECTED FILES
  autotests/kfileplacesmodeltest.cpp
  src/filewidgets/kfileplacesitem.cpp
  src/filewidgets/kfileplacesitem_p.h

To: renatoo, ngraham, #frameworks, #dolphin
Cc: elvisangelaccio, mwolff, mlaurent, #frameworks


D8434: Created 'remote' section

2017-11-01 Thread Milian Wolff
mwolff added inline comments.

INLINE COMMENTS

> kfileplacesmodeltest.cpp:784
> +// insert a new network url
> +m_places->addPlace(QStringLiteral("My Shared"), QUrl( 
> QStringLiteral("ftp://192.168.1.1/ftp;)), QString(), QString(), 
> QModelIndex());
> +

please add URLs for the following schemas, too:

smb
sftp
fish
webdav

> kfileplacesitem_p.h:48
>  PlacesType = 0,
> -RecentlySavedType = 1,
> -SearchForType = 2,
> -DevicesType = 3,
> -RemovableDevicesType = 4
> +NetworkType = 1,
> +RecentlySavedType = 2,

this should probably also be called `RemoteType` now, no?

REPOSITORY
  R241 KIO

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

To: renatoo, ngraham, #frameworks, #dolphin
Cc: elvisangelaccio, mwolff, mlaurent, #frameworks


D8434: Created 'remote' section

2017-11-01 Thread Renato Oliveira Filho
renatoo updated this revision to Diff 21694.
renatoo added a comment.


  Created unit test for remote ulrs

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8434?vs=21693=21694

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

AFFECTED FILES
  autotests/kfileplacesmodeltest.cpp
  src/filewidgets/kfileplacesitem.cpp
  src/filewidgets/kfileplacesitem_p.h

To: renatoo, ngraham, #frameworks, #dolphin
Cc: elvisangelaccio, mwolff, mlaurent, #frameworks


D7787: Set KPageListView width properly

2017-11-01 Thread Alexander Volkov
volkov added a comment.


  Could you try to test it without + 5 and with the following change applied?
  https://phabricator.kde.org/D8590 ?

REPOSITORY
  R236 KWidgetsAddons

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

To: wojnilowicz, #frameworks
Cc: volkov, cfeck, tbaumgart, #kmymoney


D8590: KPageListView: Update width on font change

2017-11-01 Thread Łukasz Wojniłowicz
wojnilowicz resigned from this revision.
wojnilowicz added a comment.


  I resign because I don't maintain KPageListView.

REPOSITORY
  R236 KWidgetsAddons

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

To: volkov, cfeck, wojnilowicz
Cc: #frameworks


D8434: Created 'remote' section

2017-11-01 Thread Nathaniel Graham
ngraham added a comment.


  +1 on Remote.
  
  We really need to remove the duplicated Places test that's the header for the 
whole widget, though.

REPOSITORY
  R241 KIO

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

To: renatoo, ngraham, #frameworks, #dolphin
Cc: elvisangelaccio, mwolff, mlaurent, #frameworks


D8434: Created 'remote' section

2017-11-01 Thread Renato Oliveira Filho
renatoo added a comment.


  In https://phabricator.kde.org/D8434#162786, @mwolff wrote:
  
  > Well, but if we use `Network` for `remote://` already, then the group 
should also have this label, no? I don't see an issue with this, really. On the 
contrary - maybe we could in the future remove the `remote://` link and let the 
category header react to a click, such that it will show you e.g. all remote 
devices. That would remove the duplication then.
  
  
  I think duplicate
  
  In https://phabricator.kde.org/D8434#162002, @ngraham wrote:
  
  > +1 for the idea! Needs more screenshots. :)
  
  
  screenshot added

REPOSITORY
  R241 KIO

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

To: renatoo, ngraham, #frameworks, #dolphin
Cc: elvisangelaccio, mwolff, mlaurent, #frameworks


D8590: KPageListView: Update width on font change

2017-11-01 Thread Alexander Volkov
volkov added a reviewer: wojnilowicz.

REPOSITORY
  R236 KWidgetsAddons

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

To: volkov, cfeck, wojnilowicz
Cc: #frameworks


D8434: Created 'remote' section

2017-11-01 Thread Renato Oliveira Filho
renatoo retitled this revision from "Created 'shared' section" to "Created 
'remote' section".
renatoo edited the summary of this revision.
renatoo edited the test plan for this revision.

REPOSITORY
  R241 KIO

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

To: renatoo, ngraham, #frameworks, #dolphin
Cc: elvisangelaccio, mwolff, mlaurent, #frameworks


D8434: Created 'shared' section

2017-11-01 Thread Renato Oliveira Filho
renatoo updated this revision to Diff 21693.
renatoo added a comment.


  Renamed section from 'sared' to 'remote'

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8434?vs=21649=21693

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

AFFECTED FILES
  autotests/kfileplacesmodeltest.cpp
  src/filewidgets/kfileplacesitem.cpp
  src/filewidgets/kfileplacesitem_p.h

To: renatoo, ngraham, #frameworks, #dolphin
Cc: elvisangelaccio, mwolff, mlaurent, #frameworks


D8590: KPageListView: Update width on font change

2017-11-01 Thread Alexander Volkov
volkov added a reviewer: cfeck.

REPOSITORY
  R236 KWidgetsAddons

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

To: volkov, cfeck
Cc: #frameworks


D8434: Created 'shared' section

2017-11-01 Thread Milian Wolff
mwolff added a comment.


  But thinking 5s more about it, I personally would say that using "Remote" for 
the category would be OK for now.

REPOSITORY
  R241 KIO

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

To: renatoo, ngraham, #frameworks, #dolphin
Cc: elvisangelaccio, mwolff, mlaurent, #frameworks


D8434: Created 'shared' section

2017-11-01 Thread Milian Wolff
mwolff added a comment.


  Well, but if we use `Network` for `remote://` already, then the group should 
also have this label, no? I don't see an issue with this, really. On the 
contrary - maybe we could in the future remove the `remote://` link and let the 
category header react to a click, such that it will show you e.g. all remote 
devices. That would remove the duplication then.

REPOSITORY
  R241 KIO

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

To: renatoo, ngraham, #frameworks, #dolphin
Cc: elvisangelaccio, mwolff, mlaurent, #frameworks


D8590: KPageListView: Update width on font change

2017-11-01 Thread Alexander Volkov
volkov created this revision.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: Frameworks.

REVISION SUMMARY
  In particular its font can be changed by Breeze style that
  modifies bold fonts to regular when it polishes list views.

REPOSITORY
  R236 KWidgetsAddons

BRANCH
  KPageListView-updateWidth

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

AFFECTED FILES
  src/kpageview_p.cpp
  src/kpageview_p.h

To: volkov
Cc: #frameworks


D8434: Created 'shared' section

2017-11-01 Thread Renato Oliveira Filho
renatoo added a comment.


  In https://phabricator.kde.org/D8434#162727, @mwolff wrote:
  
  > lgtm overall. but I wonder about the naming choice. "Shared" is confusing, 
to me personally at least. Why not call it "Remote" or "Network"? The reasoning 
is that "shared" for me is only LAN/WLAN shared stuff, while "remote://" allows 
arbitrary remote links. I use it to connect via (S)FTP to machines on the 
internet e.g.
  
  
  I first thought of using "Network" but this is already used as label for 
"remote://" url.   "Remote" can be a option since it is not used yet. But 
"Shared" came from MacOS images posted in another review, looks like is is used 
by "Finder".

REPOSITORY
  R241 KIO

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

To: renatoo, ngraham, #frameworks, #dolphin
Cc: elvisangelaccio, mwolff, mlaurent, #frameworks


D8434: Created 'shared' section

2017-11-01 Thread Elvis Angelaccio
elvisangelaccio added a comment.


  In https://phabricator.kde.org/D8434#162727, @mwolff wrote:
  
  > lgtm overall. but I wonder about the naming choice. "Shared" is confusing, 
to me personally at least. Why not call it "Remote" or "Network"? The reasoning 
is that "shared" for me is only LAN/WLAN shared stuff, while "remote://" allows 
arbitrary remote links. I use it to connect via (S)FTP to machines on the 
internet e.g.
  
  
  +1

REPOSITORY
  R241 KIO

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

To: renatoo, ngraham, #frameworks, #dolphin
Cc: elvisangelaccio, mwolff, mlaurent, #frameworks


D8566: Add API to retrieve the screen id for a screen name

2017-11-01 Thread David Edmundson
davidedmundson accepted this revision.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: amantia, #plasma, ervin, dvratil, mlaurent, hein, davidedmundson
Cc: broulik, plasma-devel, dhaumann, apol, #frameworks, ZrenBot, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart


D8434: Created 'shared' section

2017-11-01 Thread Milian Wolff
mwolff added a comment.


  lgtm overall. but I wonder about the naming choice. "Shared" is confusing, to 
me personally at least. Why not call it "Remote" or "Network"? The reasoning is 
that "shared" for me is only LAN/WLAN shared stuff, while "remote://" allows 
arbitrary remote links. I use it to connect via (S)FTP to machines on the 
internet e.g.

REPOSITORY
  R241 KIO

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

To: renatoo, ngraham, #frameworks, #dolphin
Cc: mwolff, mlaurent, #frameworks


D8566: Add API to retrieve the screen id for a screen name

2017-11-01 Thread Andras Mantia
amantia updated this revision to Diff 21682.
amantia added a comment.


  Add signals instead of a new virtual method (that is not BIC). This also 
makes us not rely on screen name, but only on plasma-internal screen id.

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8566?vs=21679=21682

BRANCH
  master

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

AFFECTED FILES
  src/plasma/corona.h

To: amantia, #plasma, ervin, dvratil, mlaurent, hein
Cc: broulik, plasma-devel, dhaumann, apol, #frameworks, ZrenBot, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart


D8566: Add API to retrieve the screen id for a screen name

2017-11-01 Thread Andras Mantia
amantia added a comment.


  @broulik Very good point, luckily I reworked it.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: amantia, #plasma, ervin, dvratil, mlaurent, hein
Cc: broulik, plasma-devel, dhaumann, apol, #frameworks, ZrenBot, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart


D8566: Add API to retrieve the screen id for a screen name

2017-11-01 Thread Kai Uwe Broulik
broulik added a comment.


  This class is exported, you cannot add a new virtual to it.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: amantia, #plasma, ervin, dvratil, mlaurent, hein
Cc: broulik, plasma-devel, dhaumann, apol, #frameworks, ZrenBot, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart


D8566: Add API to retrieve the screen id for a screen name

2017-11-01 Thread Andras Mantia
amantia updated this revision to Diff 21679.
amantia added a comment.
Restricted Application added a project: Plasma.
Restricted Application added a subscriber: plasma-devel.


  Fix API docs text

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8566?vs=21617=21679

BRANCH
  master

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

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

To: amantia, #plasma, ervin, dvratil, mlaurent, hein
Cc: plasma-devel, dhaumann, apol, #frameworks, ZrenBot, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, mart


D8566: Add API to retrieve the screen id for a screen name

2017-11-01 Thread Dominik Haumann
dhaumann added inline comments.

INLINE COMMENTS

> ervin wrote in corona.h:223
> "if there is no such screen" I guess?

And it is @return and not @returns.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: amantia, #plasma, ervin, dvratil, mlaurent, hein
Cc: dhaumann, apol, #frameworks