[Differential] [Commented On] D4814: kio_help: use doOutputBuffer and simplify unicodeError

[Luigi Toscano]

ltoscano added a comment.


  Thank you! And lesson learned: change the message here too, as arcanist did 
not update it.

REPOSITORY
  R241 KIO

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

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

To: ltoscano, dfaure
Cc: #documentation, #frameworks

[Differential] [Closed] D4814: kio_help: use doOutputBuffer and simplify unicodeError

[Luigi Toscano]

This revision was automatically updated to reflect the committed changes.
ltoscano marked 3 inline comments as done.
Closed by commit R241:96200470ada8: kio_help: use doOutputBuffer and simplify 
unicodeError (authored by ltoscano).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D4814?vs=11874=11877

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

AFFECTED FILES
  src/ioslaves/help/kio_help.cpp
  src/ioslaves/help/kio_help.h

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

To: ltoscano, dfaure
Cc: #documentation, #frameworks

[Differential] [Updated] D4813: Add function to extract a single file

[Luigi Toscano]

ltoscano marked 3 inline comments as done.
ltoscano added a comment.


  Thank you! Comments addressed (with a bit more comment than null)

REPOSITORY
  R238 KDocTools

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

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

To: ltoscano, dfaure
Cc: dfaure, #frameworks, #documentation

[Differential] [Closed] D4813: Add function to extract a single file

[Luigi Toscano]

This revision was automatically updated to reflect the committed changes.
Closed by commit R238:be23cd457327: Add function to extract a single file 
(authored by ltoscano).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D4813?vs=11873=11876#toc

REPOSITORY
  R238 KDocTools

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D4813?vs=11873=11876

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

AFFECTED FILES
  src/xslt.cpp
  src/xslt.h

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

To: ltoscano, dfaure
Cc: dfaure, #frameworks, #documentation

[Differential] [Accepted] D4813: Add function to extract a single file

[David Faure]

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


  Looks good, just some nitpicks. Feel free to push directly.

INLINE COMMENTS

> xslt.cpp:365
> +if (index == -1) {
> +if (filename == QStringLiteral("index.html")) {
> +return fromUnicode(content);

QLatin1String would be better (because it's a comparison, and there's a 
QLatin1String overload for this purpose)

> xslt.cpp:368
> +} else {
> +return QByteArray(); // null
> +}

I like the value added by the comment :-)

> xslt.h:17
>  
> +/*
> + * Extract the content of a single file from the content string

I think doxygen needs /** here, no?

REPOSITORY
  R238 KDocTools

BRANCH
  master

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

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

To: ltoscano, dfaure
Cc: dfaure, #frameworks, #documentation

[Differential] [Updated, 37 lines] D4814: kio_help: use doOutputBuffer and simplify unicodeError

[Luigi Toscano]

ltoscano updated this revision to Diff 11874.
ltoscano added a comment.


  Use the updated name of the new function

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D4814?vs=11866=11874

BRANCH
  master

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

AFFECTED FILES
  src/ioslaves/help/kio_help.cpp
  src/ioslaves/help/kio_help.h

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

To: ltoscano, dfaure
Cc: #documentation, #frameworks

[Differential] [Updated, 22 lines] D4813: Add function to extract a single file

[Luigi Toscano]

ltoscano updated this revision to Diff 11873.
ltoscano added a comment.


  More meaningful name for the new function

REPOSITORY
  R238 KDocTools

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D4813?vs=11863=11873

BRANCH
  master

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

AFFECTED FILES
  src/xslt.cpp
  src/xslt.h

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

To: ltoscano
Cc: dfaure, #frameworks, #documentation

[Differential] [Closed] D2075: Fix bug in kfiledialog.cpp that causes crashing when native widgets are used.

[Albert Astals Cid]

This revision was automatically updated to reflect the committed changes.
Closed by commit R239:1ba106517481: Fix bug in kfiledialog.cpp that causes 
crashing when native widgets are used. (authored by jonathans, committed by 
aacid).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D2075?vs=10021=11871#toc

REPOSITORY
  R239 KDELibs4Support

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D2075?vs=10021=11871

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

AFFECTED FILES
  src/kio/kfiledialog.cpp

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

To: jonathans, #frameworks, dfaure, kfunk
Cc: kfunk, aacid

[Differential] [Commented On] D4813: Add function to extract a single file

[David Faure]

dfaure added a comment.


  The code looks ok, I'm just not sure this is "nice" exported API though.  
("do", no documentation, ...)

REPOSITORY
  R238 KDocTools

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

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

To: ltoscano
Cc: dfaure, #frameworks, #documentation

[Differential] [Commented On] D4814: kio_help: use doOutputBuffer and simplify unicodeError

[Luigi Toscano]

ltoscano added a comment.


  I consider this an implicit acceptance for the parent review 
https://phabricator.kde.org/D4813 then :)

REPOSITORY
  R241 KIO

BRANCH
  master

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

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

To: ltoscano, dfaure
Cc: #documentation, #frameworks

[Differential] [Accepted] D4814: kio_help: use doOutputBuffer and simplify unicodeError

[David Faure]

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

REPOSITORY
  R241 KIO

BRANCH
  master

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

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

To: ltoscano, dfaure
Cc: #documentation, #frameworks

Re: Review Request 129663: Don't break accelerators in KToolBar

[David Faure]

> On Feb. 12, 2017, 8:13 a.m., David Faure wrote:
> > "Qt doesn't do this" = Qt doesn't strip '&' from action texts in toolbars? 
> > I can't confirm that.
> > 
> > With this patch and my kxmlgui patch reverted, I get "Open File (F)" and 
> > "Print (P)" in the konqueror toolbar, and Alt+F opens the file menu and 
> > Alt+P does nothing. Accelerators do not work.
> > 
> > I see what you mean with the Control button in dolphin, but that is a bit 
> > different, it's a QToolButton created by code directly, not a QAction.
> > 
> > So I stand by what I said: Qt strips '&' from *action* texts in toolbars 
> > (and it happens in QAction::iconText).
> 
> Martin Tobias Holmedahl Sandsmark wrote:
> I'm very confused. Why does the accelerator for "Sort By" show up and 
> work here? https://iskrembilen.com/screenshots/dolphinaccel.png
> 
> And even with my patch here, no accelerators show up for anything in 
> Konqueror for me. Which patch by you did you revert?

I'm just as confused ;)  I edited toolbars in dolphin to add "Sort by" to the 
toolbar, and even when pressing Alt, there is no accelerator shown in the 
toolbar.

Qt 5.8 from git, dolphin 16.12.2 from current Applications/16.12 branch, breeze 
widget style from Plasma/5.9 branch.

qt_strippedText is in qaction.cpp since Qt 5.0 so surely we both have it. How 
is it possible that in your case QAction::iconText doesn't call qt_strippedText 
? How about we compare debug output ?

After applying this patch to Qt http://www.davidfaure.fr/2017/qaction.cpp.diff
if I run `dolphin |& grep sort` I get

QAction::iconText: ICON "sort" stripping "Sort By" => "Sort By"
QAction::iconText: ICON "sort" stripping " By" => "Sort By"

Do you get the same?

(this happens twice because KAcceleratorManager adds the '&' at runtime; but it 
gets stripped anyhow in iconText(), clearly)

QToolButton::setDefaultAction is what calls iconText() in order to set the text 
on the toolbutton.
qtbase commit a09b41b changed QToolButton::setDefaultAction, but that is in 
5.5.1 and later, surely you have this commit too?

The revert I mentionned wasn't in kxmlgui (oops) but my suggested fix in 
kwidgetaddons.


- David


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


On Dec. 17, 2016, 12:23 p.m., Martin Tobias Holmedahl Sandsmark wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129663/
> ---
> 
> (Updated Dec. 17, 2016, 12:23 p.m.)
> 
> 
> Review request for KDE Frameworks, David Faure and Chusslove Illich.
> 
> 
> Repository: kxmlgui
> 
> 
> Description
> ---
> 
> Don't try to strip out accelerators in the KToolBar event handler. It makes 
> no sense to me, potentially creates an endless repaint loop and fights with 
> KAcceleratorManager which will constantly re-add accelerators.
> 
> 
> Diffs
> -
> 
>   src/ktoolbar.cpp 31be9b0 
> 
> Diff: https://git.reviewboard.kde.org/r/129663/diff/
> 
> 
> Testing
> ---
> 
> With this patch not only the control button in Dolphin has an accelerator.
> 
> 
> Thanks,
> 
> Martin Tobias Holmedahl Sandsmark
> 
>

Re: Review Request 129741: Add renaming capability to ioslaves

[Elvis Angelaccio]

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

(Updated Feb. 26, 2017, 11:10 p.m.)


Status
--

This change has been discarded.


Review request for KDE Frameworks, David Faure and Emmanuel Pescosta.


Repository: kio


Description
---

An ioslave protocol might support moving files but not "renaming" them (rename 
here means "the F2 shortcut in filemanagers"). trash:/ is a good example.

The new `renaming` field defaults to `moving`, so that we don't have to add 
renaming=true to every protocol file out there.


Diffs
-

  src/core/kfileitemlistproperties.h 2b4a5b33166513493e6499e6479a04965a895b57 
  src/core/kfileitemlistproperties.cpp 5c6e6bba693f8b9bfc942ff39bee5d24f159fd7f 
  src/core/kprotocolinfo.cpp 0290c63b37a45a22995238f9cfcc11b8334d339c 
  src/core/kprotocolinfo_p.h 8d05bd194fdaa7b7e7552e0d1d22bf16b28ffbc1 
  src/core/kprotocolmanager.h 13b8c0756f8e355b1ec84cdf1c44086f41fa05c5 
  src/core/kprotocolmanager.cpp 9a0a96fe749a11c66a22bb3eff62d0601e3b7b36 
  src/ioslaves/trash/tests/testtrash.cpp 
67a6130e7c86af00e596dc439125c29eb74fc99f 
  src/ioslaves/trash/trash.json d7dc03eb073c7bfdde3c7eebfbac144ad62964fe 
  src/protocoltojson/main.cpp 0bf3c7062d076412c779f6cae25a98e1b2c61be4 

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


Testing
---

Using the new `supportsRenaming()` api from Dolphin, F2 in Trash is now 
disabled. More context in https://git.reviewboard.kde.org/r/129714


Thanks,

Elvis Angelaccio

Re: Review Request 129741: Add renaming capability to ioslaves

[Elvis Angelaccio]

> On Jan. 2, 2017, 9:38 a.m., David Faure wrote:
> > Renaming is really a special case of moving. Saying that "kio_trash doesn't 
> > support renaming" is correct but only a partial truth. It also doesn't 
> > support moving from trash:/ to trash:/subdir/. So it would be more correct 
> > to say that kio_trash supports moving trash-to-file and file-to-trash but 
> > not trash-to-trash.
> > 
> > So it seems to me that this patch models the wrong thing, and is likely to 
> > give us further trouble down the line.
> > 
> > I wonder if we could invent something more dynamic than the .protocol keys. 
> > An additional way to talk to a slave and ask if a specific operation (for 
> > specific URLs) is implemented. Something like KIO::CapabilityJob *job = 
> > KIO::capability(Move, url1, url2); connect; and in the slot, enable/disable 
> > the action accordingly. By default this would just use the information from 
> > the .protocol files, but in addition a new SlaveBase method (fake-virtual 
> > until KF6, using virtual_hook) would allow slaves to answer the query with 
> > more precision, depending on the actual URLs. What do you think?
> > 
> > Of course the alternative (which is actually simpler short term) is to 
> > implement renaming in kio_trash ;)
> > Actual renaming should be easy, moving between subdirs is a bit more tricky 
> > but doable too. I can take a look at that next weekend.
> 
> Elvis Angelaccio wrote:
> > Renaming is really a special case of moving. Saying that "kio_trash 
> doesn't support renaming" is correct but only a partial truth. It also 
> doesn't support moving from trash:/ to trash:/subdir/. So it would be more 
> correct to say that kio_trash supports moving trash-to-file and file-to-trash 
> but not trash-to-trash.
> 
> Good point...
> > Something like KIO::CapabilityJob *job = KIO::capability(Move, url1, 
> url2); connect; and in the slot, enable/disable the action accordingly.
> 
> This would be more flexible indeed. On the other hand, it's more verbose 
> and low-level. Also, for the "renaming" case one needs a way to make up url2 
> (url1 would just be the selected url in the dolphin view).
> > Of course the alternative (which is actually simpler short term) is to 
> implement renaming in kio_trash ;)
> 
> Definitely :)
> All right, let's discuss this again later, waiting for news from 
> kio_trash.
> 
> David Faure wrote:
> Renaming implemented in kio_trash (for toplevel entries) 
> https://commits.kde.org/kio/20f0b84f51ff7f0767da118de79eda28af091ec9
> 
> Elvis Angelaccio wrote:
> Thanks! Works fine from kioclient, hower it doesn't from Dolphin, which 
> doesn't prepend the 0- prefix to the destination url (so it uses 
> `trash:/destination` instead of `trash:/0-destination`). I wonder how this 
> could be fixed...
> 
> David Faure wrote:
> Good point. Does it work better now after 
> https://commits.kde.org/kio/9afce8395c6604697379046581bce24786a1bcb7 ?
> 
> Elvis Angelaccio wrote:
> Yes, now I can rename from dolphin. There is a (malformed url) error if I 
> try to restore a file that I just renamed, but it does work after if I reload 
> the view. Anyway I think I can discard this patch now!
> 
> David Faure wrote:
> Oh, I see. Dolphin (via KIO) asks to rename trash:/0-foo to trash:/bar, 
> and the resulting file has a URL of trash:/0-bar.
> So the KFileItem has the wrong URL, anything you do with it will fail 
> (e.g. renaming it again). Argh.
> 
> Ah, the KDirNotify signal emits the wrong URL, that's why.
> https://commits.kde.org/kio/a1c040a43a19bc896c7a2f19703ce43c4c9b9423 
> fixes this.
> 
> Of course there's always one bug left: now the view shows 0-newname 
> instead of newname. It seems the item text is simply extracted from the URL 
> rather than UDS_DISPLAY_NAME being used. But now that looks like a KIO (or 
> Dolphin) bug, not a kio_trash issue.

I can confirm. Smells like a bug in the dolphin's KFileItemModel...
Thanks again, discarding this patch.


- Elvis


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


On Jan. 1, 2017, 11:01 p.m., Elvis Angelaccio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129741/
> ---
> 
> (Updated Jan. 1, 2017, 11:01 p.m.)
> 
> 
> Review request for KDE Frameworks, David Faure and Emmanuel Pescosta.
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> An ioslave protocol might support moving files but not "renaming" them 
> (rename here means "the F2 shortcut in filemanagers"). trash:/ is a good 
> example.
> 
> The new `renaming` field defaults to `moving`, so that we don't have 

[Differential] [Updated, 37 lines] D4814: kio_help: use doOutputBuffer and simplify unicodeError

[Luigi Toscano]

ltoscano updated this revision to Diff 11866.
ltoscano added a comment.


  Follow the hints: rename unicodeError(), consistenly close data()

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D4814?vs=11864=11866

BRANCH
  master

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

AFFECTED FILES
  src/ioslaves/help/kio_help.cpp
  src/ioslaves/help/kio_help.h

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

To: ltoscano, dfaure
Cc: #documentation, #frameworks

[Differential] [Commented On] D4814: kio_help: use doOutputBuffer and simplify unicodeError

[Luigi Toscano]

ltoscano added a comment.


  For the record, I'm trying to reduce the number of functions used because the 
next step (which I hope to send for review before 5.32, even if the time is 
short) is finally exporting a proper public .so from KDocTools.

REPOSITORY
  R241 KIO

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

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

To: ltoscano, dfaure
Cc: #documentation, #frameworks

[Differential] [Commented On] D4814: kio_help: use doOutputBuffer and simplify unicodeError

[Luigi Toscano]

ltoscano added inline comments.

INLINE COMMENTS

> dfaure wrote in kio_help.cpp:136
> While at it: this change makes the method name quite strange. Rename to 
> sendError ?

Probably historical reasons from when UTF-8 was not "da" codec, and I'm not 
sure I want to dig into the history

> dfaure wrote in kio_help.cpp:138
> Here the call to data() is not followed by a data(QByteArray()) 

Right, I simply followed the old behavior, but it's easy to fix.

> dfaure wrote in kio_help.cpp:344
> ... while here the call to data() is followed by data(empty bytearray), as 
> per the kio SlaveBase docu.
> 
> I suggest making it consistent (the best solution depends on what the other 
> calls to unicodeError() look like)

Other calls are inside get() and the corresponding usage of data does not 
include data(QByteArray()), so I think I would change emitFile to always add 
that line at the end.

REPOSITORY
  R241 KIO

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

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

To: ltoscano, dfaure
Cc: #documentation, #frameworks

[Differential] [Commented On] D4814: kio_help: use doOutputBuffer and simplify unicodeError

[David Faure]

dfaure added inline comments.

INLINE COMMENTS

> kio_help.cpp:136
>  
>  void HelpProtocol::unicodeError(const QString )
>  {

While at it: this change makes the method name quite strange. Rename to 
sendError ?

> kio_help.cpp:138
>  {
> -#ifdef Q_OS_WIN
> -QString encoding = "UTF-8";
> -#else
> -QString encoding = QTextCodec::codecForLocale()->name();
> -#endif
> -data(fromUnicode(QStringLiteral(
> - " content=\"text/html; charset=%1\">\n%2").arg(encoding, 
> t.toHtmlEscaped(;
> +data(QStringLiteral(
> + " charset=UTF-8\">\n%1").arg(t.toHtmlEscaped()).toUtf8());

Here the call to data() is not followed by a data(QByteArray()) 

> kio_help.cpp:344
> +data(result);
> +data(QByteArray());
>  }

... while here the call to data() is followed by data(empty bytearray), as per 
the kio SlaveBase docu.

I suggest making it consistent (the best solution depends on what the other 
calls to unicodeError() look like)

REPOSITORY
  R241 KIO

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

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

To: ltoscano, dfaure
Cc: #documentation, #frameworks

Re: Review Request 129741: Add renaming capability to ioslaves

[David Faure]

> On Jan. 2, 2017, 9:38 a.m., David Faure wrote:
> > Renaming is really a special case of moving. Saying that "kio_trash doesn't 
> > support renaming" is correct but only a partial truth. It also doesn't 
> > support moving from trash:/ to trash:/subdir/. So it would be more correct 
> > to say that kio_trash supports moving trash-to-file and file-to-trash but 
> > not trash-to-trash.
> > 
> > So it seems to me that this patch models the wrong thing, and is likely to 
> > give us further trouble down the line.
> > 
> > I wonder if we could invent something more dynamic than the .protocol keys. 
> > An additional way to talk to a slave and ask if a specific operation (for 
> > specific URLs) is implemented. Something like KIO::CapabilityJob *job = 
> > KIO::capability(Move, url1, url2); connect; and in the slot, enable/disable 
> > the action accordingly. By default this would just use the information from 
> > the .protocol files, but in addition a new SlaveBase method (fake-virtual 
> > until KF6, using virtual_hook) would allow slaves to answer the query with 
> > more precision, depending on the actual URLs. What do you think?
> > 
> > Of course the alternative (which is actually simpler short term) is to 
> > implement renaming in kio_trash ;)
> > Actual renaming should be easy, moving between subdirs is a bit more tricky 
> > but doable too. I can take a look at that next weekend.
> 
> Elvis Angelaccio wrote:
> > Renaming is really a special case of moving. Saying that "kio_trash 
> doesn't support renaming" is correct but only a partial truth. It also 
> doesn't support moving from trash:/ to trash:/subdir/. So it would be more 
> correct to say that kio_trash supports moving trash-to-file and file-to-trash 
> but not trash-to-trash.
> 
> Good point...
> > Something like KIO::CapabilityJob *job = KIO::capability(Move, url1, 
> url2); connect; and in the slot, enable/disable the action accordingly.
> 
> This would be more flexible indeed. On the other hand, it's more verbose 
> and low-level. Also, for the "renaming" case one needs a way to make up url2 
> (url1 would just be the selected url in the dolphin view).
> > Of course the alternative (which is actually simpler short term) is to 
> implement renaming in kio_trash ;)
> 
> Definitely :)
> All right, let's discuss this again later, waiting for news from 
> kio_trash.
> 
> David Faure wrote:
> Renaming implemented in kio_trash (for toplevel entries) 
> https://commits.kde.org/kio/20f0b84f51ff7f0767da118de79eda28af091ec9
> 
> Elvis Angelaccio wrote:
> Thanks! Works fine from kioclient, hower it doesn't from Dolphin, which 
> doesn't prepend the 0- prefix to the destination url (so it uses 
> `trash:/destination` instead of `trash:/0-destination`). I wonder how this 
> could be fixed...
> 
> David Faure wrote:
> Good point. Does it work better now after 
> https://commits.kde.org/kio/9afce8395c6604697379046581bce24786a1bcb7 ?
> 
> Elvis Angelaccio wrote:
> Yes, now I can rename from dolphin. There is a (malformed url) error if I 
> try to restore a file that I just renamed, but it does work after if I reload 
> the view. Anyway I think I can discard this patch now!

Oh, I see. Dolphin (via KIO) asks to rename trash:/0-foo to trash:/bar, and the 
resulting file has a URL of trash:/0-bar.
So the KFileItem has the wrong URL, anything you do with it will fail (e.g. 
renaming it again). Argh.

Ah, the KDirNotify signal emits the wrong URL, that's why.
https://commits.kde.org/kio/a1c040a43a19bc896c7a2f19703ce43c4c9b9423 fixes this.

Of course there's always one bug left: now the view shows 0-newname instead of 
newname. It seems the item text is simply extracted from the URL rather than 
UDS_DISPLAY_NAME being used. But now that looks like a KIO (or Dolphin) bug, 
not a kio_trash issue.


- David


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


On Jan. 1, 2017, 11:01 p.m., Elvis Angelaccio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129741/
> ---
> 
> (Updated Jan. 1, 2017, 11:01 p.m.)
> 
> 
> Review request for KDE Frameworks, David Faure and Emmanuel Pescosta.
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> An ioslave protocol might support moving files but not "renaming" them 
> (rename here means "the F2 shortcut in filemanagers"). trash:/ is a good 
> example.
> 
> The new `renaming` field defaults to `moving`, so that we don't have to add 
> renaming=true to every protocol file out there.
> 
> 
> Diffs
> -
> 
>   src/core/kfileitemlistproperties.h 2b4a5b33166513493e6499e6479a04965a895b57 
>   

[Differential] [Updated] D4814: kio_help: use doOutputBuffer and simplify unicodeError

[Luigi Toscano]

ltoscano added a reviewer: dfaure.
ltoscano added a project: Documentation.
ltoscano added a subscriber: Documentation.

REPOSITORY
  R241 KIO

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

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

To: ltoscano, dfaure
Cc: #documentation, #frameworks

[Differential] [Updated] D4814: kio_help: use doOutputBuffer and simplify unicodeError

[Luigi Toscano]

ltoscano added a dependency: D4813: Add function to extract a single file.

REPOSITORY
  R241 KIO

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

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

To: ltoscano
Cc: #frameworks

[Differential] [Updated] D4813: Add function to extract a single file

[Luigi Toscano]

ltoscano added a dependent revision: D4814: kio_help: use doOutputBuffer and 
simplify unicodeError.

REPOSITORY
  R238 KDocTools

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

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

To: ltoscano
Cc: #frameworks, #documentation

[Differential] [Request, 27 lines] D4814: kio_help: use doOutputBuffer and simplify unicodeError

[Luigi Toscano]

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

REVISION SUMMARY
  - use the new doOutputBuffer from kdoctools and reduce the number of 
lower-level functions from kdoctools used by kio;
  - unicodeError: force utf-8 as codec; no reason to use a local codec, 
browsers should be able to render it. Also, remove the last usage of 
fromUnicode().

TEST PLAN
  Compiles, the code branches previously handled directly and
  now managed by doOutputBuffer still works.

REPOSITORY
  R241 KIO

BRANCH
  master

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

AFFECTED FILES
  src/ioslaves/help/kio_help.cpp

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

To: ltoscano
Cc: #frameworks

[Differential] [Request, 17 lines] D4813: Add function to extract a single file

[Luigi Toscano]

ltoscano created this revision.
Restricted Application added projects: Frameworks, Documentation.
Restricted Application added subscribers: Documentation, Frameworks.

REVISION SUMMARY
  Extract a single file from a QString which contains the generated
  content.
  The usage of this function by kio_help reduces the number of the
  functions from xslt.* used by KIO.

TEST PLAN
  Compiles, no functional changes (tested with a dependent change in KIO).

REPOSITORY
  R238 KDocTools

BRANCH
  master

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

AFFECTED FILES
  src/xslt.cpp
  src/xslt.h

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

To: ltoscano
Cc: #frameworks, #documentation

[Differential] [Requested Changes] D4689: IconItem: Add roundToIconSize property

[Sebastian Kügler]

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


  I think a problem with using roundToIconSize as isolated property is that it 
really isn't. It has intended effects on the sizing of the item, but the 
current version of the patch doesn't reflect that. I think it needs to trigger 
a series of size change signals. See my comments inline.

INLINE COMMENTS

> iconitemtest.cpp:524
> +
> +item->setProperty("roundToIconSize", false);
> +

Just checking the values here is not enough, the property change results in 
changes in paintedWidth, but we're currently not signalling that these props 
have changed. See also my comment below.

> iconitem.cpp:313
> +}
> +void IconItem::setRoundToIconSize(bool roundToIconSize)
> +{

Changing roundToIconSize may change the size of the icon, so should we trigger 
size changes (paintedWidth / paintedHeight / boundingRect likely? Perhaps 
others.) and re-rendering here as well? Right now, judging from the code, 
changing this property mid-flight is broken. We may even have to go as far as 
loading a new pixmap (in loadPixmap() from a quick glance).

Please also add unit tests for the effects on the iconitem's size properties.

> iconitem.h:147
>  
> +bool isRoundingToIconSize() const;
> +void setRoundToIconSize(bool roundToIconSize);

bool roundToIconSize() const;

we generally don't use "is" in new code where we can avoid it, and the ing 
makes this even more inconsistent. I'd much prefer the above suggestion.

Please also add api docs, at least for new code.

REPOSITORY
  R242 Plasma Framework (Library)

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

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

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

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

[Valerio Pilo]

vpilo added a comment.


  In https://phabricator.kde.org/D4799#90169, @dfaure wrote:
  
  > About the code in kded that calls ksplash: that code is obsolete and 
currently only kept for compatibility reasons, see 
https://git.reviewboard.kde.org/r/129010/
  >  IOW you can ignore that code completely.
  
  
  OK, thanks :)
  
  In https://phabricator.kde.org/D4799#90165, @broulik wrote:
  
  > Most are not, however. PowerDevil, for instance, actually delays its 
notifications (e.g. you startup with a low battery) until a notification 
service registers itself to avoid the embarrassing popup ontop of KSplash while 
still showing the notification soon after logging in. Almost anything else 
("You are now connected to network X") is pointless, and I hate this Yakuake 
notification when logging in.
  
  
  All notifications can be disabled if the user finds them useless and/or 
annoying - not a reason to prevent them from showing at all by means of 
frameworks, though, IMHO. And we can't expect the applications to all do what 
PowerDevil does. Unless we make this change happen (with another trigger to 
decide whether the splash is being shown).

REPOSITORY
  R289 KNotifications

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

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

To: vpilo
Cc: dfaure, broulik, graesslin, mck182, #frameworks

Re: Review Request 122183: [KUnitConversion] Currency: Fetch the currency file properly

[Vishesh Handa]

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

(Updated Feb. 26, 2017, 8:47 p.m.)


Status
--

This change has been discarded.


Review request for KDE Frameworks.


Bugs: 340819
https://bugs.kde.org/show_bug.cgi?id=340819


Repository: kunitconversion


Description
---

Currency: Fetch the currency file properly

Properly run an event loop and wait for the file to be fetched.

Also add a test to make sure currency conversion is working.

This patch also contains -
* https://git.reviewboard.kde.org/r/122182/
* https://git.reviewboard.kde.org/r/122181/
* https://git.reviewboard.kde.org/r/122180/

This is because reviewboard refuses to upload only a part of the diff. Please 
only look at currency.cpp w.r.t the EventLoop.


Diffs
-

  autotests/convertertest.h 8129a48 
  autotests/convertertest.cpp ae4298e 
  src/currency.cpp 715233c 

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


Testing
---

Test now passes.


Thanks,

Vishesh Handa

Re: Review Request 123032: Search for public dep in KPeople's cmake config

[David Faure]

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



No, it's whitespace because a reviewboard upgrade a year ago or so lost some 
patches, which had to be re-uploaded. It looks like this is one of them too. 
It's not necessarily committed. Hard to know for sure, but at least the summary 
doesn't match a commit in kpeople.

- David Faure


On March 18, 2015, 6:30 p.m., Hrvoje Senjan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/123032/
> ---
> 
> (Updated March 18, 2015, 6:30 p.m.)
> 
> 
> Review request for KDE Frameworks and Aleix Pol Gonzalez.
> 
> 
> Repository: kpeople
> 
> 
> Description
> ---
> 
> Both KF5::PeopleBackend and KF5::PeopleWidgets need Qt5Widgets publicly, so 
> search them.
> 
> 
> Diffs
> -
> 
>   KF5PeopleConfig.cmake.in 05ae340 
> 
> Diff: https://git.reviewboard.kde.org/r/123032/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Hrvoje Senjan
> 
>

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

[David Faure]

dfaure added a comment.


  About the code in kded that calls ksplash: that code is obsolete and 
currently only kept for compatibility reasons, see 
https://git.reviewboard.kde.org/r/129010/
  IOW you can ignore that code completely.

REPOSITORY
  R289 KNotifications

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

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

To: vpilo
Cc: dfaure, broulik, graesslin, mck182, #frameworks

[Differential] [Closed] D4802: [KTextEditor] Remember file type set by user over sessions

[John Salatas]

This revision was automatically updated to reflect the committed changes.
Closed by commit R39:ba03878378a4: Remember file type set by user over sessions 
(authored by jsalatas).

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D4802?vs=11836=11862

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

AFFECTED FILES
  src/document/katedocument.cpp

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

To: jsalatas, #frameworks, #ktexteditor, cullmann
Cc: dhaumann, cullmann, kwrite-devel

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

[Kai Uwe Broulik]

broulik added a comment.


  > It is a bug in ksplash that the popups are visible at all.
  
  Fallback for KNotification is a KPassivePopup which is unredirect I think? 
Ksplash, in contrast to the splash screen, doesn't continuously re-raise itself 
and KWin doesn't do anything special to it either. (apart from some special 
cases in various effects)
  
  > Some popups might be useful, some are very useful
  
  Most are not, however. PowerDevil, for instance, actually delays its 
notifications (e.g. you startup with a low battery) until a notification 
service registers itself to avoid the embarrassing popup ontop of KSplash while 
still showing the notification soon after logging in. Almost anything else 
("You are now connected to network X") is pointless, and I hate this Yakuake 
notification when logging in.

REPOSITORY
  R289 KNotifications

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

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

To: vpilo
Cc: broulik, graesslin, mck182, #frameworks

[Differential] [Commented On] D4810: ecm_install_po_files_as_qm: filter the .po files

[Luigi Toscano]

ltoscano added a comment.


  In https://phabricator.kde.org/D4810#90158, @aacid wrote:
  
  > What's the use case?
  
  
  Point this macro and ki18n_install() (which may require a similar change) in 
the same directory. I understand that it's not stricly needed (they can be two 
separate directories).

REPOSITORY
  R240 Extra CMake Modules

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

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

To: ltoscano
Cc: aacid, #frameworks, #build_system

[Differential] [Commented On] D4810: ecm_install_po_files_as_qm: filter the .po files

[Albert Astals Cid]

aacid added a comment.


  What's the use case?

REPOSITORY
  R240 Extra CMake Modules

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

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

To: ltoscano
Cc: aacid, #frameworks, #build_system

[Differential] [Accepted] D4802: [KTextEditor] Remember file type set by user over sessions

[Christoph Cullmann]

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


  Yeah, good catch!

REPOSITORY
  R39 KTextEditor

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

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

To: jsalatas, #frameworks, #ktexteditor, cullmann
Cc: dhaumann, cullmann, kwrite-devel

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

[Valerio Pilo]

vpilo added a comment.


  @graesslin I disagree. Some popups might be useful, some are very useful; we 
should aim to never drop any. Just as an example, the popup with the Yakuake 
console toggle key is pretty fundamental to be reminded of, the first few times 
you start it up.
  
  It's all about saving users from annoyances. Papercuts, in Canonical terms.
  
  @mck182 - I think your approach of taking them in, and replaying them at the 
end, is the best option I can see from my inexperienced POV.

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

[Differential] [Commented On] D4711: Ungrab mouse on menu close

[Anthony Fieroni]

anthonyfieroni added a comment.


  Yep, same faulty beharior present in all Qt apps, Qupzilla, QtCreator, etc.

REPOSITORY
  R242 Plasma Framework (Library)

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

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

To: anthonyfieroni, #plasma, broulik, mart
Cc: mvourlakos, plasma-devel, #frameworks, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol

Re: kwin requires qt5.7.0

[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 129741: Add renaming capability to ioslaves

[Elvis Angelaccio]

> On Jan. 2, 2017, 9:38 a.m., David Faure wrote:
> > Renaming is really a special case of moving. Saying that "kio_trash doesn't 
> > support renaming" is correct but only a partial truth. It also doesn't 
> > support moving from trash:/ to trash:/subdir/. So it would be more correct 
> > to say that kio_trash supports moving trash-to-file and file-to-trash but 
> > not trash-to-trash.
> > 
> > So it seems to me that this patch models the wrong thing, and is likely to 
> > give us further trouble down the line.
> > 
> > I wonder if we could invent something more dynamic than the .protocol keys. 
> > An additional way to talk to a slave and ask if a specific operation (for 
> > specific URLs) is implemented. Something like KIO::CapabilityJob *job = 
> > KIO::capability(Move, url1, url2); connect; and in the slot, enable/disable 
> > the action accordingly. By default this would just use the information from 
> > the .protocol files, but in addition a new SlaveBase method (fake-virtual 
> > until KF6, using virtual_hook) would allow slaves to answer the query with 
> > more precision, depending on the actual URLs. What do you think?
> > 
> > Of course the alternative (which is actually simpler short term) is to 
> > implement renaming in kio_trash ;)
> > Actual renaming should be easy, moving between subdirs is a bit more tricky 
> > but doable too. I can take a look at that next weekend.
> 
> Elvis Angelaccio wrote:
> > Renaming is really a special case of moving. Saying that "kio_trash 
> doesn't support renaming" is correct but only a partial truth. It also 
> doesn't support moving from trash:/ to trash:/subdir/. So it would be more 
> correct to say that kio_trash supports moving trash-to-file and file-to-trash 
> but not trash-to-trash.
> 
> Good point...
> > Something like KIO::CapabilityJob *job = KIO::capability(Move, url1, 
> url2); connect; and in the slot, enable/disable the action accordingly.
> 
> This would be more flexible indeed. On the other hand, it's more verbose 
> and low-level. Also, for the "renaming" case one needs a way to make up url2 
> (url1 would just be the selected url in the dolphin view).
> > Of course the alternative (which is actually simpler short term) is to 
> implement renaming in kio_trash ;)
> 
> Definitely :)
> All right, let's discuss this again later, waiting for news from 
> kio_trash.
> 
> David Faure wrote:
> Renaming implemented in kio_trash (for toplevel entries) 
> https://commits.kde.org/kio/20f0b84f51ff7f0767da118de79eda28af091ec9
> 
> Elvis Angelaccio wrote:
> Thanks! Works fine from kioclient, hower it doesn't from Dolphin, which 
> doesn't prepend the 0- prefix to the destination url (so it uses 
> `trash:/destination` instead of `trash:/0-destination`). I wonder how this 
> could be fixed...
> 
> David Faure wrote:
> Good point. Does it work better now after 
> https://commits.kde.org/kio/9afce8395c6604697379046581bce24786a1bcb7 ?

Yes, now I can rename from dolphin. There is a (malformed url) error if I try 
to restore a file that I just renamed, but it does work after if I reload the 
view. Anyway I think I can discard this patch now!


- Elvis


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


On Jan. 1, 2017, 11:01 p.m., Elvis Angelaccio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129741/
> ---
> 
> (Updated Jan. 1, 2017, 11:01 p.m.)
> 
> 
> Review request for KDE Frameworks, David Faure and Emmanuel Pescosta.
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> An ioslave protocol might support moving files but not "renaming" them 
> (rename here means "the F2 shortcut in filemanagers"). trash:/ is a good 
> example.
> 
> The new `renaming` field defaults to `moving`, so that we don't have to add 
> renaming=true to every protocol file out there.
> 
> 
> Diffs
> -
> 
>   src/core/kfileitemlistproperties.h 2b4a5b33166513493e6499e6479a04965a895b57 
>   src/core/kfileitemlistproperties.cpp 
> 5c6e6bba693f8b9bfc942ff39bee5d24f159fd7f 
>   src/core/kprotocolinfo.cpp 0290c63b37a45a22995238f9cfcc11b8334d339c 
>   src/core/kprotocolinfo_p.h 8d05bd194fdaa7b7e7552e0d1d22bf16b28ffbc1 
>   src/core/kprotocolmanager.h 13b8c0756f8e355b1ec84cdf1c44086f41fa05c5 
>   src/core/kprotocolmanager.cpp 9a0a96fe749a11c66a22bb3eff62d0601e3b7b36 
>   src/ioslaves/trash/tests/testtrash.cpp 
> 67a6130e7c86af00e596dc439125c29eb74fc99f 
>   src/ioslaves/trash/trash.json d7dc03eb073c7bfdde3c7eebfbac144ad62964fe 
>   src/protocoltojson/main.cpp 0bf3c7062d076412c779f6cae25a98e1b2c61be4 
> 
> Diff: https://git.reviewboard.kde.org/r/129741/diff/
> 
> 
> 

[Differential] [Commented On] D4797: [ToolButtonStyle] Use pure colors and no frame as background in flat mode

[Kai Uwe Broulik]

broulik added a comment.


  > So changing only the theme would mean to change the svg of `ButtonStyle`, 
which is not desirable.
  
  Then we need to introduce new elements in `widgets/button.svg` or a new 
`widget/toolbutton.svg` for `ToolButton` which is used and falls back to the 
old behavior if not present (to not break compat with other themes). 
`QToolButton` is QtWidgets which has nothing to do with QML or Plasma styles.

REPOSITORY
  R242 Plasma Framework (Library)

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

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

To: subdiff, #plasma
Cc: ltoscano, broulik, hpereiradacosta, plasma-devel, #frameworks, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

[Differential] [Commented On] D4797: [ToolButtonStyle] Use pure colors and no frame as background in flat mode

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  > Also I tested it now with the Oxygen and United themes, and `QToolButton` 
has still the same flat frame line in highlight color around it in when hovered 
like in Breeze, so it seems to be independent of the theme in `QToolButton` as 
well. In the Oxygen case it is though different looking in the pressed and 
checked state. Here it has a "depth" effect to it (or it's only the frame line 
getting bigger). Not sure how we can replicate that.
  
  Speaking for the widgets only (QToolButton), the fact that mouse-over looks 
similar (and, if you look closely, not identical in fact) is just a coincidence.
  This is handled by the widget style and is not hard coded
  Check with fusion style for instance (namely: dolphin -style fusion)
  
  for the QML side of things I have no idea.

REPOSITORY
  R242 Plasma Framework (Library)

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

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

To: subdiff, #plasma
Cc: ltoscano, broulik, hpereiradacosta, plasma-devel, #frameworks, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

Re: kwin requires qt5.7.0

[Allen Winter]

On Sunday, February 26, 2017 06:05:13 PM Luigi Toscano wrote:
> Allen Winter ha scritto:
> > 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.
> 
> Well, that's a question for the Plasma list (where kwin belongs).
> Frameworks and Plasma (and the other applications) may have different
> requirements (as it is right now).
> 
de facto, Qt5.7 it is then.
starting over ...

Re: kwin requires qt5.7.0

[Luigi Toscano]

Allen Winter ha scritto:
> 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.

Well, that's a question for the Plasma list (where kwin belongs).
Frameworks and Plasma (and the other applications) may have different
requirements (as it is right now).

-- 
Luigi

[Differential] [Commented On] D4797: [ToolButtonStyle] Use pure colors and no frame as background in flat mode

[Roman Gilg]

subdiff added a comment.


  In https://phabricator.kde.org/D4797#90126, @broulik wrote:
  
  > Design looks ok but still you can't change `ToolButtonStyle`, instead, the 
Breeze Plasma theme needs to be changed.
  
  
  Sorry, I didn't quite get it the last time you mentioned it. I think I 
understand it now. You mean it will make other themes look like Breeze, when we 
insert the code in `ToolButtonStyle`, correct? But in this case take a look at 
the current `ToolButtonStyle` code: It takes `ButtonStyle`'s svg frame directly 
and uses it for its hovered/pressed state. So changing anything in the theme 
would mean to change the svg of `ButtonStyle`, which is not desirable.
  
  Also I tested it now with the Oxygen and United themes, and `QToolButton` has 
still the same flat frame line in highlight color around it in when hovered 
like in Breeze, so it seems to be independent of the theme in `QToolButton` as 
well. In the Oxygen case it is though different looking in the pressed and 
checked state. Here it has a "depth" effect to it (or it's only the frame line 
getting bigger). Not sure how we can replicate that.

REPOSITORY
  R242 Plasma Framework (Library)

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

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

To: subdiff, #plasma
Cc: ltoscano, broulik, hpereiradacosta, plasma-devel, #frameworks, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

kwin requires qt5.7.0

[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.

when running CMake on kwin I get:
CMake Error at CMakeLists.txt:20 (find_package):
  Could not find a configuration file for package "Qt5" that is compatible
  with requested version "5.7.0".

  The following configuration files were considered but not accepted:

/usr/lib64/cmake/Qt5/Qt5Config.cmake, version: 5.6.2

[Differential] [Changed Subscribers] D4802: [KTextEditor] Remember file type set by user over sessions

[Dominik Haumann]

dhaumann added subscribers: cullmann, dhaumann.
dhaumann added a comment.


  Does this fix https://bugs.kde.org/show_bug.cgi?id=358281 ?
  
  In general, I think this looks good and your video also demonstrates this 
nicely.
  Still, a second ok would be nice.
  @cullmann Does this also look good to you?

REPOSITORY
  R39 KTextEditor

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

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

To: jsalatas, #frameworks, #ktexteditor
Cc: dhaumann, cullmann, kwrite-devel

[Differential] [Commented On] D4797: [ToolButtonStyle] Use pure colors and no frame as background in flat mode

[Kai Uwe Broulik]

broulik added a comment.


  Design looks ok but still you can't change `ToolButtonStyle`, instead, the 
Breeze Plasma theme needs to be changed.

REPOSITORY
  R242 Plasma Framework (Library)

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

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

To: subdiff, #plasma
Cc: ltoscano, broulik, hpereiradacosta, plasma-devel, #frameworks, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

[Differential] [Commented On] D4797: [ToolButtonStyle] Use pure colors and no frame as background in flat mode

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  + 1 for me. Looks good!

REPOSITORY
  R242 Plasma Framework (Library)

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

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

To: subdiff, #plasma
Cc: ltoscano, broulik, hpereiradacosta, plasma-devel, #frameworks, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

[Differential] [Request, 148 lines] D4810: ecm_install_po_files_as_qm: filter the .po files

[Luigi Toscano]

ltoscano created this revision.
Restricted Application added projects: Frameworks, Build System.
Restricted Application added subscribers: Build System, Frameworks.

REVISION SUMMARY
  Add a new parameter to ecm_install_po_files_as_qm which allows users
  to filter the .po files handled by the function.
  The parameter is a glob-like pattern which is matched against the name
  of the files.

TEST PLAN
  Compiles, the old tests still pass and the test added for the new case
  pass as well.

REPOSITORY
  R240 Extra CMake Modules

BRANCH
  qm_install_withpattern

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

AFFECTED FILES
  modules/ECMPoQmTools.cmake
  tests/ECMPoQmToolsTest/CMakeLists.txt
  tests/ECMPoQmToolsTest/check.cmake.in
  tests/ECMPoQmToolsTest/pofilter/es/install-pattern1.po
  tests/ECMPoQmToolsTest/pofilter/es/install-pattern2.po
  tests/ECMPoQmToolsTest/pofilter/es/install-test.po
  tests/ECMPoQmToolsTest/pofilter/fr/install-pattern1.po
  tests/ECMPoQmToolsTest/pofilter/fr/install-pattern2.po
  tests/ECMPoQmToolsTest/pofilter/fr/install-test.po
  tests/ECMPoQmToolsTest/pofilter/no-po/placeholder
  tests/ECMPoQmToolsTest/pofilter/should-not-be-installed

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

To: ltoscano
Cc: #frameworks, #build_system

[Differential] [Updated, 169 lines] D4797: [ToolButtonStyle] Use pure colors and no frame as background in flat mode

[Roman Gilg]

subdiff updated this revision to Diff 11853.
subdiff added a comment.


  Replicate the design of QToolButton (used for example in the tool bars of 
System Settings and Dolphin).
  
  F2617274: Spectacle.oB6068.png 

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D4797?vs=11829=11853

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

AFFECTED FILES
  src/declarativeimports/plasmastyle/ToolButtonStyle.qml

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

To: subdiff, #plasma
Cc: ltoscano, broulik, hpereiradacosta, plasma-devel, #frameworks, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

EBN News: SVN Projects Removed

[Allen Winter]

[resending with my kde address]

Howdy,

On the EBN we are no longer checking-out projects from subversion.
This means if you have a project hanging around in SVN you will no longer see 
its apidox, Krazy reports, etc.

-Allen

Re: Review Request 129741: Add renaming capability to ioslaves

[David Faure]

> On Jan. 2, 2017, 9:38 a.m., David Faure wrote:
> > Renaming is really a special case of moving. Saying that "kio_trash doesn't 
> > support renaming" is correct but only a partial truth. It also doesn't 
> > support moving from trash:/ to trash:/subdir/. So it would be more correct 
> > to say that kio_trash supports moving trash-to-file and file-to-trash but 
> > not trash-to-trash.
> > 
> > So it seems to me that this patch models the wrong thing, and is likely to 
> > give us further trouble down the line.
> > 
> > I wonder if we could invent something more dynamic than the .protocol keys. 
> > An additional way to talk to a slave and ask if a specific operation (for 
> > specific URLs) is implemented. Something like KIO::CapabilityJob *job = 
> > KIO::capability(Move, url1, url2); connect; and in the slot, enable/disable 
> > the action accordingly. By default this would just use the information from 
> > the .protocol files, but in addition a new SlaveBase method (fake-virtual 
> > until KF6, using virtual_hook) would allow slaves to answer the query with 
> > more precision, depending on the actual URLs. What do you think?
> > 
> > Of course the alternative (which is actually simpler short term) is to 
> > implement renaming in kio_trash ;)
> > Actual renaming should be easy, moving between subdirs is a bit more tricky 
> > but doable too. I can take a look at that next weekend.
> 
> Elvis Angelaccio wrote:
> > Renaming is really a special case of moving. Saying that "kio_trash 
> doesn't support renaming" is correct but only a partial truth. It also 
> doesn't support moving from trash:/ to trash:/subdir/. So it would be more 
> correct to say that kio_trash supports moving trash-to-file and file-to-trash 
> but not trash-to-trash.
> 
> Good point...
> > Something like KIO::CapabilityJob *job = KIO::capability(Move, url1, 
> url2); connect; and in the slot, enable/disable the action accordingly.
> 
> This would be more flexible indeed. On the other hand, it's more verbose 
> and low-level. Also, for the "renaming" case one needs a way to make up url2 
> (url1 would just be the selected url in the dolphin view).
> > Of course the alternative (which is actually simpler short term) is to 
> implement renaming in kio_trash ;)
> 
> Definitely :)
> All right, let's discuss this again later, waiting for news from 
> kio_trash.
> 
> David Faure wrote:
> Renaming implemented in kio_trash (for toplevel entries) 
> https://commits.kde.org/kio/20f0b84f51ff7f0767da118de79eda28af091ec9
> 
> Elvis Angelaccio wrote:
> Thanks! Works fine from kioclient, hower it doesn't from Dolphin, which 
> doesn't prepend the 0- prefix to the destination url (so it uses 
> `trash:/destination` instead of `trash:/0-destination`). I wonder how this 
> could be fixed...

Good point. Does it work better now after 
https://commits.kde.org/kio/9afce8395c6604697379046581bce24786a1bcb7 ?


- David


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


On Jan. 1, 2017, 11:01 p.m., Elvis Angelaccio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129741/
> ---
> 
> (Updated Jan. 1, 2017, 11:01 p.m.)
> 
> 
> Review request for KDE Frameworks, David Faure and Emmanuel Pescosta.
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> An ioslave protocol might support moving files but not "renaming" them 
> (rename here means "the F2 shortcut in filemanagers"). trash:/ is a good 
> example.
> 
> The new `renaming` field defaults to `moving`, so that we don't have to add 
> renaming=true to every protocol file out there.
> 
> 
> Diffs
> -
> 
>   src/core/kfileitemlistproperties.h 2b4a5b33166513493e6499e6479a04965a895b57 
>   src/core/kfileitemlistproperties.cpp 
> 5c6e6bba693f8b9bfc942ff39bee5d24f159fd7f 
>   src/core/kprotocolinfo.cpp 0290c63b37a45a22995238f9cfcc11b8334d339c 
>   src/core/kprotocolinfo_p.h 8d05bd194fdaa7b7e7552e0d1d22bf16b28ffbc1 
>   src/core/kprotocolmanager.h 13b8c0756f8e355b1ec84cdf1c44086f41fa05c5 
>   src/core/kprotocolmanager.cpp 9a0a96fe749a11c66a22bb3eff62d0601e3b7b36 
>   src/ioslaves/trash/tests/testtrash.cpp 
> 67a6130e7c86af00e596dc439125c29eb74fc99f 
>   src/ioslaves/trash/trash.json d7dc03eb073c7bfdde3c7eebfbac144ad62964fe 
>   src/protocoltojson/main.cpp 0bf3c7062d076412c779f6cae25a98e1b2c61be4 
> 
> Diff: https://git.reviewboard.kde.org/r/129741/diff/
> 
> 
> Testing
> ---
> 
> Using the new `supportsRenaming()` api from Dolphin, F2 in Trash is now 
> disabled. More context in https://git.reviewboard.kde.org/r/129714
> 
> 
> Thanks,
> 
> Elvis Angelaccio
> 
>

[Differential] [Commented On] D4797: [ToolButtonStyle] Use pure colors and no frame as background in flat mode

[Kai Uwe Broulik]

broulik added a comment.


  > Would you support the idea of using the KToolBar ToolButton design for the 
QML ToolButton aswell?
  
  Good idea.

REPOSITORY
  R242 Plasma Framework (Library)

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

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

To: subdiff, #plasma
Cc: ltoscano, broulik, hpereiradacosta, plasma-devel, #frameworks, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

[Differential] [Commented On] D4797: [ToolButtonStyle] Use pure colors and no frame as background in flat mode

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  Hi Roman,
  
  > Would you support the idea of using the KToolBar ToolButton design for the 
QML ToolButton aswell? It looks way better in my opinion and it would make the 
use of tool buttons more consistent.
  
  Yes, I would support this. It would be more consistent with QWidget.

REPOSITORY
  R242 Plasma Framework (Library)

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

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

To: subdiff, #plasma
Cc: ltoscano, broulik, hpereiradacosta, plasma-devel, #frameworks, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

[Differential] [Commented On] D4797: [ToolButtonStyle] Use pure colors and no frame as background in flat mode

[Luigi Toscano]

ltoscano added a comment.


  In https://phabricator.kde.org/D4797#90068, @subdiff wrote:
  
  > This looks weird, because painting a whole button is not a small hint for 
the user that he can interact with the element but looks more like a completely 
new independent button suddenly being created.
  
  
  As a user (but old user, ok), I never thought about this. It is just the 
button raising when it is possible to activate it.

REPOSITORY
  R242 Plasma Framework (Library)

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

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

To: subdiff, #plasma
Cc: ltoscano, broulik, hpereiradacosta, plasma-devel, #frameworks, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

[Differential] [Commented On] D4797: [ToolButtonStyle] Use pure colors and no frame as background in flat mode

[Roman Gilg]

subdiff added a comment.


  I see where you're coming from. You're right, that the rounded edges don't 
fit so well to other Plasma element. So I would try to find another design, 
because I still think the current ToolButton is ugly. Let me make it more 
clear, what I dislike about the current (flat) ToolButton design: It just takes 
the Button design for its hover/clicked state. This looks weird, because 
painting a whole button is not a small hint for the user that he can interact 
with the element but looks more like a completely new independent button 
suddenly being created.
  
  Besides that, did you notice that we have two different ToolButton designs in 
our Frameworks? The above one for QML in Plasma Frameworks and one for QWidgets 
in KToolBar. Take a look at Dolphin for example:
  
  F2616299: video3.mp4 
  
  Would you support the idea of using the KToolBar ToolButton design for the 
QML ToolButton aswell? It looks way better in my opinion and would make the use 
of tool buttons more consistent.

REPOSITORY
  R242 Plasma Framework (Library)

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

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

To: subdiff, #plasma
Cc: broulik, hpereiradacosta, plasma-devel, #frameworks, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol

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

[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
> 
>

Re: Review Request 129663: Don't break accelerators in KToolBar

[Martin Tobias Holmedahl Sandsmark]

> On Feb. 12, 2017, 8:13 a.m., David Faure wrote:
> > "Qt doesn't do this" = Qt doesn't strip '&' from action texts in toolbars? 
> > I can't confirm that.
> > 
> > With this patch and my kxmlgui patch reverted, I get "Open File (F)" and 
> > "Print (P)" in the konqueror toolbar, and Alt+F opens the file menu and 
> > Alt+P does nothing. Accelerators do not work.
> > 
> > I see what you mean with the Control button in dolphin, but that is a bit 
> > different, it's a QToolButton created by code directly, not a QAction.
> > 
> > So I stand by what I said: Qt strips '&' from *action* texts in toolbars 
> > (and it happens in QAction::iconText).

I'm very confused. Why does the accelerator for "Sort By" show up and work 
here? https://iskrembilen.com/screenshots/dolphinaccel.png

And even with my patch here, no accelerators show up for anything in Konqueror 
for me. Which patch by you did you revert?


- Martin Tobias Holmedahl


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


On Dec. 17, 2016, 12:23 p.m., Martin Tobias Holmedahl Sandsmark wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129663/
> ---
> 
> (Updated Dec. 17, 2016, 12:23 p.m.)
> 
> 
> Review request for KDE Frameworks, David Faure and Chusslove Illich.
> 
> 
> Repository: kxmlgui
> 
> 
> Description
> ---
> 
> Don't try to strip out accelerators in the KToolBar event handler. It makes 
> no sense to me, potentially creates an endless repaint loop and fights with 
> KAcceleratorManager which will constantly re-add accelerators.
> 
> 
> Diffs
> -
> 
>   src/ktoolbar.cpp 31be9b0 
> 
> Diff: https://git.reviewboard.kde.org/r/129663/diff/
> 
> 
> Testing
> ---
> 
> With this patch not only the control button in Dolphin has an accelerator.
> 
> 
> Thanks,
> 
> Martin Tobias Holmedahl Sandsmark
> 
>

[Differential] [Updated, 56 lines] D4689: IconItem: Add roundToIconSize property

[David Rosca]

drosca updated this revision to Diff 11845.
drosca added a comment.


  Add default is true

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D4689?vs=11551=11845

BRANCH
  arcpatch-D4689

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

AFFECTED FILES
  autotests/iconitemtest.cpp
  autotests/iconitemtest.h
  src/declarativeimports/core/iconitem.cpp
  src/declarativeimports/core/iconitem.h

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

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

[Differential] [Commented On] D4797: [ToolButtonStyle] Use pure colors and no frame as background in flat mode

[Kai Uwe Broulik]

broulik added a comment.


  I agree with Hugo. I also never liked the faint blue press effect of buttons 
that was introduced in the Breeze revamp in 5.5 to begin with, having that as 
normal state now is a no-go for me.
  
  Also, you can't just randomly change the behavior of ToolButtonStyle, 
instead, if we were to do this, the Breeze Plasma style should be adjusted, 
introducing a dedicated widget and/or new hints if needed, instead of the 
button component.

REPOSITORY
  R242 Plasma Framework (Library)

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

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

To: subdiff, #plasma
Cc: broulik, hpereiradacosta, plasma-devel, #frameworks, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol

[Differential] [Commented On] D4797: [ToolButtonStyle] Use pure colors and no frame as background in flat mode

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  Hello Roman, 
  I disagree that the current toolbutton design "doesn't fit the overall 
design". It is (to me at least) consistent with the widget style (in e.g. 
toolbars), and all the other "squarish" elements of breeze.
  
  On the contrary, I would find the new design, with its large rounding radius, 
inconsistent with the rest, since (unless i am mistaken) such a large radius is 
used nowhere else, either in the widget or the qml style. 
  So that would be a -1, sorry.
  That does not mean that the calendar plasmoid should not be fixed, to prevent 
the items overlap.

REPOSITORY
  R242 Plasma Framework (Library)

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

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

To: subdiff, #plasma
Cc: hpereiradacosta, plasma-devel, #frameworks, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol