Review Request 126610: kwidgetitemdelegate: properly cleanup widgets on index removal

[Pino Toscano]

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

Review request for KDE Frameworks.


Repository: kitemviews


Description
---

When indexes are removed and their widgets deleted, the event filter on each 
widget is not removed, leading to the "you should not delete widgets 
manually"-alike warning.

Add an helper forgetAbout() function which performs all the actions done 
per-widget before deleting each, additionally removing also the event filter.


Diffs
-

  src/kwidgetitemdelegate.cpp 779dc2a8a57148fb37f1f5a7194bc9656cb305a4 
  src/kwidgetitemdelegatepool.cpp e916dddad8be56bb803e241da43d8cbe7a171ec3 
  src/kwidgetitemdelegatepool_p.h 401fe193b0954d6c7c721503d4657b7f08e9fd2e 

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


Testing
---

A sample application with widgets for items in the model, removing indexes: no 
more warning at removal time.


Thanks,

Pino Toscano

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

Re: Review Request 120139: kill warning

[Pino Toscano]

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


IMHO the warning is not totally pointless, and it should be better debugged why 
it is triggered.

Anyway, I just posted https://git.reviewboard.kde.org/r/126610/ to avoid 
triggering it on manual index removal.

- Pino Toscano


On Set. 11, 2014, 10:48 a.m., Sune Vuorela wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/120139/
> ---
> 
> (Updated Set. 11, 2014, 10:48 a.m.)
> 
> 
> Review request for KDE Frameworks, Andreas Hartmetz and Stephen Kelly.
> 
> 
> Repository: kitemviews
> 
> 
> Description
> ---
> 
> The warninng is even triggered by from internal KWidgetItemDelegatePrivate, 
> so seems to be wrong
> 
> Try out the kwidgetitemdelegate test application
> 
> 
> Diffs
> -
> 
>   src/kwidgetitemdelegatepool.cpp d61802e 
> 
> Diff: https://git.reviewboard.kde.org/r/120139/diff/
> 
> 
> Testing
> ---
> 
> Warning not printed.
> 
> 
> Thanks,
> 
> Sune Vuorela
> 
>

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

Re: Review Request 126610: kwidgetitemdelegate: properly cleanup widgets on index removal

[Pino Toscano]

> On Gen. 2, 2016, 7:31 p.m., Sune Vuorela wrote:
> > Looks much better than my attempt over in 
> > https://git.reviewboard.kde.org/r/120139

Note my patch does not remove the warnings on close, although I found out how 
to avoid them: to the KWidgetItemDelegate construr, either pass a null parent, 
or the view->viewport().


- Pino


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


On Gen. 2, 2016, 7:24 p.m., Pino Toscano wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126610/
> ---
> 
> (Updated Gen. 2, 2016, 7:24 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kitemviews
> 
> 
> Description
> ---
> 
> When indexes are removed and their widgets deleted, the event filter on each 
> widget is not removed, leading to the "you should not delete widgets 
> manually"-alike warning.
> 
> Add an helper forgetAbout() function which performs all the actions done 
> per-widget before deleting each, additionally removing also the event filter.
> 
> 
> Diffs
> -
> 
>   src/kwidgetitemdelegate.cpp 779dc2a8a57148fb37f1f5a7194bc9656cb305a4 
>   src/kwidgetitemdelegatepool.cpp e916dddad8be56bb803e241da43d8cbe7a171ec3 
>   src/kwidgetitemdelegatepool_p.h 401fe193b0954d6c7c721503d4657b7f08e9fd2e 
> 
> Diff: https://git.reviewboard.kde.org/r/126610/diff/
> 
> 
> Testing
> ---
> 
> A sample application with widgets for items in the model, removing indexes: 
> no more warning at removal time.
> 
> 
> Thanks,
> 
> Pino Toscano
> 
>

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

Re: Menu/Command capitalization

[Pino Toscano]

Hi,

In data giovedì 7 gennaio 2016 22:42:18, Jaroslaw Staniek ha scritto:
> Not too long ago MS Windows has moved from "Title Capitalization" to
> "Sentence capitalization" for menus and commands:
> https://msdn.microsoft.com/en-us/library/dn742392.aspx
> 
> What can we do and should we do something about this?

Nothing IMHO -- see below.

> - For everyone else perhaps it's better to offer something officially
> or else some people would fork translations or avoid using KDE's code.

You don't need to fork anything, just provide a "translation" on top of
the untranslated strings.

> - Is automated conversion for capitalization a reasonable approach?
> Runtime or script-based generation.

Definitely not: please never ever mess up with user-visible strings.

> - Do we have checks for semantic strings and capitalization? If so
> would they need updates, or?

By hand, when somebody spots a mistake and fixes it.

> - Some (future?) KF5 users would target Windows expecting that the
> resulting apps look and feel largely native.

> [...]

> - Mac OS X and iOS is consistent with KDE capitalization:
>   
> https://developer.apple.com/library/mac/documentation/UserExperience/Conceptual/OSXHIGuidelines/TerminologyWording.html#//apple_ref/doc/uid/2957-CH15-SW4

To simplify the situation:
- right now we are like OS-A but different than OS-B
- with your proposal, we would be like OS-B but different than OS-A
So, there would be always a class of users feeling "native" (regarding
the string capitalization),  and another one feeling "alien".

Sorry, but I don't see how mass-changing English strings, to end up in
the very same situation (1 like, 1 different) wrt other OSes. At least
for me, it has been hard enough to keep KDE applications following our
style in the last 10+ years, I don't want to start over again.

-- 
Pino Toscano

signature.asc
Description: This is a digitally signed message part.
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Menu/Command capitalization

[Pino Toscano]

In data venerdì 8 gennaio 2016 12:51:35, Jaroslaw Staniek ha scritto:
> On 8 January 2016 at 00:28, Pino Toscano  wrote:
> > Hi,
> >
> > In data giovedì 7 gennaio 2016 22:42:18, Jaroslaw Staniek ha scritto:
> >> Not too long ago MS Windows has moved from "Title Capitalization" to
> >> "Sentence capitalization" for menus and commands:
> >> https://msdn.microsoft.com/en-us/library/dn742392.aspx
> >>
> >> What can we do and should we do something about this?
> >
> > Nothing IMHO -- see below.
> >
> >> - For everyone else perhaps it's better to offer something officially
> >> or else some people would fork translations or avoid using KDE's code.
> >
> > You don't need to fork anything, just provide a "translation" on top of
> > the untranslated strings.
> 
> Thanks for the notes, Pino.
> That was not the exact question. What I mean is who provides the translation.

Just like any language among the ones in our repositories: if there's
interest, somebody will step up and do the work.

> If not KDE, then the whole thing can only be forked unspecified number
> of times. In extreme case, if "Foo Bar" is replaced with "Foo bar" _in
> the code_, then also the code is also forked. Not good.

I'm not sure why you're going that far in thinking about forking, only
for this. Forking has a cost, and if all you need is just provide
different texts to user-visible strings, then maintaining a separate
translation is a lot easier to maintain; this works even in the worst
case, that is when such translation is maintained privately and not
shared among the KDE community.

> Regarding the number of locales, I see things do not look that bad: we
> have en_US == C and en_GB only.
> Please correct me here.
> 
> My idea is to provide extra English translations that are variants of
> the originals but with sentence capitalization.
> Ideally that would be the two extra:
> - en_GBS (based on en_GB)
> - en_USS (based on en_US)
> 
> (S suffix == Sentence)
> 
> I believe both can be largely generated on demand.

Demand from who?

> Technically at Qt level: Having the en_*S locales, our apps/libs would
> need to use them under certain conditions (e.g. on Android, Windows)
> instead of the original locales. Do nothing e.g. on Mac OS X.

-1

> >> - Is automated conversion for capitalization a reasonable approach?
> >> Runtime or script-based generation.
> >
> > Definitely not: please never ever mess up with user-visible strings.
> 
> Please see the above, maybe what I propose is nice enough, better than
> half of the strings in app in one style (KF5 strings), another in
> other style (app strings). Even the same title could have two forms in
> the same app.
> The same button's text.

This is totally different issue, that is lack of periodic style reviews
in our libraries and applications; we do have one style, we just don't
"enforce" it properly everywhere.

> >> - Some (future?) KF5 users would target Windows expecting that the
> >> resulting apps look and feel largely native.
> >
> >> [...]
> >
> >> - Mac OS X and iOS is consistent with KDE capitalization:
> >>   
> >> https://developer.apple.com/library/mac/documentation/UserExperience/Conceptual/OSXHIGuidelines/TerminologyWording.html#//apple_ref/doc/uid/2957-CH15-SW4
> >
> > To simplify the situation:
> > - right now we are like OS-A but different than OS-B
> > - with your proposal, we would be like OS-B but different than OS-A
> > So, there would be always a class of users feeling "native" (regarding
> > the string capitalization),  and another one feeling "alien".
> 
> I've done a small research for you in the original post. There are not
> just *two* OSes.
> OS-A == Android, Jolla, Windows Phone, Windows Desktop
> OS-B == Mac OS X, iOS

Sure, two classes of OSes. Regardless, what I said above still stands.

> And KDE, GNOME (etc): undecided but actually both are OS-B because
> their legacy is follows: KDE - Windows (before the world moved to
> Sentence cap.), GNOME: Mac OS X.
> 
> So neither OS-A nor OS-B is a small group. If anything, OS-A is bigger
> in number of potential users (I don't count things like KDE Plasma but
> apps).

I don't see how "there are more Windows desktops than KDE" made us do
style choices in the past that do something else than what Windows does.

> > Sorry, but I don't see how mass-changing English strings, to end up in
> > the very same situation (1 like, 1 different) wrt other OSes. At least
> > for me, it has been hard

Review Request 126936: help: fix garbage sent when serving static files

[Pino Toscano]

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

Review request for KDE Frameworks, David Faure and Luigi Toscano.


Repository: kio


Description
---

Switch from a `QByteArray` to a simplier stack-based char array, using the 
actual number of bytes read when sending the data using `data()` (reusing the 
memory from the array).

This removes the garbage sent when serving static files, i.e. those stored in 
directories and not as part of a `.cache.bz2` file.


Diffs
-

  src/ioslaves/help/kio_help.cpp 7b97599a250f113e25b9c6efe238afab08b3fe2c 

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


Testing
---

The data sent for static files now is the actual bytes of the files, without 
garbage at the end.


Thanks,

Pino Toscano

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

Re: Review Request 126936: help: fix garbage sent when serving static files

[Pino Toscano]

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

(Updated Jan. 31, 2016, 11:41 a.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks, David Faure and Luigi Toscano.


Changes
---

Submitted with commit 3642b5284e65d94db22a86d52e710dd60e15b230 by Pino Toscano 
to branch master.


Repository: kio


Description
---

Switch from a `QByteArray` to a simplier stack-based char array, using the 
actual number of bytes read when sending the data using `data()` (reusing the 
memory from the array).

This removes the garbage sent when serving static files, i.e. those stored in 
directories and not as part of a `.cache.bz2` file.


Diffs
-

  src/ioslaves/help/kio_help.cpp 7b97599a250f113e25b9c6efe238afab08b3fe2c 

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


Testing
---

The data sent for static files now is the actual bytes of the files, without 
garbage at the end.


Thanks,

Pino Toscano

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

Review Request 127002: Add KLocalizedString::getLanguages()

[Pino Toscano]

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

Review request for KDE Frameworks and Chusslove Illich.


Repository: ki18n


Description
---

Easy way to get the ordered list of languages where translations are looked 
into.

This makes it possible to lookup other kind of translated resources using the 
same list of priorities of languages.

(Note: I'm not sure about which name fits better, i.e. `languages()` or 
`getLanguages()`; I chose the latter as the former could be mistaken as 
non-static method of `KLocalizedString`.)


Diffs
-

  src/klocalizedstring.h 87030a443d36c3e9ca57e1e61ba1cba13f39bbee 
  src/klocalizedstring.cpp 20b2bd46d6748b540e4a0bc9b3c3168e55cf47f3 

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


Testing
---

Builds, tests pass.


Thanks,

Pino Toscano

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

Re: Review Request 127002: Add KLocalizedString::getLanguages()

[Pino Toscano]

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

(Updated Feb. 7, 2016, 4:28 p.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks and Chusslove Illich.


Changes
---

Submitted with commit 84359916472cf872271703272d87bfe73a4fa69b by Pino Toscano 
to branch master.


Repository: ki18n


Description
---

Easy way to get the ordered list of languages where translations are looked 
into.

This makes it possible to lookup other kind of translated resources using the 
same list of priorities of languages.

(Note: I'm not sure about which name fits better, i.e. `languages()` or 
`getLanguages()`; I chose the latter as the former could be mistaken as 
non-static method of `KLocalizedString`.)


Diffs
-

  src/klocalizedstring.h 87030a443d36c3e9ca57e1e61ba1cba13f39bbee 
  src/klocalizedstring.cpp 20b2bd46d6748b540e4a0bc9b3c3168e55cf47f3 

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


Testing
---

Builds, tests pass.


Thanks,

Pino Toscano

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

Review Request 127106: applications.menu: remove references to unused categories

[Pino Toscano]

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

Review request for KDE Frameworks.


Repository: kservice


Description
---

Stop handling few categories long deprecated (replaced by standard categories):

- `X-KDE-KDevelopIDE` (with associated, and not existing, 
`kf5-development-kdevelop.directory`) -- KDevelop does not use it anymore since 
long
- `X-KDE-Edu-Language` -- replaced by Languages
- `X-KDE-KidsGame` -- replaced by KidsGame

Nothing on kde.org references them, and neither anything currently packaged in 
Debian.


Diffs
-

  src/applications.menu f2d52538c4ed6dc39d04a10e7c072cb060b3fd2b 

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


Testing
---


Thanks,

Pino Toscano

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

Re: Review Request 126610: kwidgetitemdelegate: properly cleanup widgets on index removal

[Pino Toscano]

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



ping

- Pino Toscano


On Mar. 28, 2016, 1:39 p.m., Pino Toscano wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126610/
> ---
> 
> (Updated Mar. 28, 2016, 1:39 p.m.)
> 
> 
> Review request for KDE Frameworks and Stephen Kelly.
> 
> 
> Repository: kitemviews
> 
> 
> Description
> ---
> 
> When indexes are removed and their widgets deleted, the event filter on each 
> widget is not removed, leading to the "you should not delete widgets 
> manually"-alike warning.
> 
> Add an helper forgetAbout() function which performs all the actions done 
> per-widget before deleting each, additionally removing also the event filter.
> 
> 
> Diffs
> -
> 
>   src/kwidgetitemdelegate.cpp 779dc2a8a57148fb37f1f5a7194bc9656cb305a4 
>   src/kwidgetitemdelegatepool.cpp e916dddad8be56bb803e241da43d8cbe7a171ec3 
>   src/kwidgetitemdelegatepool_p.h 401fe193b0954d6c7c721503d4657b7f08e9fd2e 
> 
> Diff: https://git.reviewboard.kde.org/r/126610/diff/
> 
> 
> Testing
> ---
> 
> A sample application with widgets for items in the model, removing indexes: 
> no more warning at removal time.
> 
> 
> Thanks,
> 
> Pino Toscano
> 
>

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

Review Request 127972: Always update the Predicate parser from y/l sources

[Pino Toscano]

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

Review request for KDE Frameworks and Lukáš Tinkl.


Repository: solid


Description
---

Turn Flex and Bison into required build dependencies, and use them to always 
regenerate at build time the Predicate parser. This ensures that the parser 
does not rot, and there is no more need to rely on autogenerated sources added 
statically among the others.

Second commit: remove old generated files of Predicate parser


Diffs
-

  CMakeLists.txt 763e09cfeeebdc9e42b68e8ab6c9e29c54d3e741 
  src/solid/CMakeLists.txt f2b43b27cb47531ed57b2eccafad8e67951b56b9 
  src/solid/devices/CMakeLists.txt 9271ae1e36b67b112be54a6ff9c6fb76a8a0a824 
  src/solid/devices/predicate_lexer.c 3b5a0b90907baf1cd2631da4de650ec153d0f642 
  src/solid/devices/predicate_parser.h 68e25070d498f5a635489af51f4b772c5f374108 
  src/solid/devices/predicate_parser.c 6d35ff25f001a43cbfecacc11e7d7591bb4808f9 

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


Testing
---

Builds fine with flex 2.6.0 and bison 3.0.4; `make test` passes too.


Thanks,

Pino Toscano

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

Re: Review Request 127972: Always update the Predicate parser from y/l sources

[Pino Toscano]

> On May 19, 2016, 10:30 p.m., Aleix Pol Gonzalez wrote:
> > This has been like this for years. Has any of the premises changed?
> 
> Michael Pyne wrote:
> I think it's a good idea, if only to keep up with changes to the 
> generated sources to account for things like undefined behavior fixes, fixes 
> for new compiler warnings, etc. Some of our users will build with updated 
> bison/flex (and I believe they're both required for KHTML still already) so 
> it's best to limit the number of different versions of generated parsers out 
> there, IMHO.

Before this RR, I fixed with commit b3d52004b0f1af0abd0a2ca907a7c857293dce36 
the predicate grammar, which was producing a non-compiling source; also, the 
current `UpdateSolidPredicateParser` rule is broken. These led me thinking that 
nobody really run the rule in years, which is not good.

Additionally, generated sources added statically in the source tree is really a 
bad idea.


- Pino


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


On May 19, 2016, 10:19 p.m., Pino Toscano wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127972/
> ---
> 
> (Updated May 19, 2016, 10:19 p.m.)
> 
> 
> Review request for KDE Frameworks and Lukáš Tinkl.
> 
> 
> Repository: solid
> 
> 
> Description
> ---
> 
> Turn Flex and Bison into required build dependencies, and use them to always 
> regenerate at build time the Predicate parser. This ensures that the parser 
> does not rot, and there is no more need to rely on autogenerated sources 
> added statically among the others.
> 
> Second commit: remove old generated files of Predicate parser
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt 763e09cfeeebdc9e42b68e8ab6c9e29c54d3e741 
>   src/solid/CMakeLists.txt f2b43b27cb47531ed57b2eccafad8e67951b56b9 
>   src/solid/devices/CMakeLists.txt 9271ae1e36b67b112be54a6ff9c6fb76a8a0a824 
>   src/solid/devices/predicate_lexer.c 
> 3b5a0b90907baf1cd2631da4de650ec153d0f642 
>   src/solid/devices/predicate_parser.h 
> 68e25070d498f5a635489af51f4b772c5f374108 
>   src/solid/devices/predicate_parser.c 
> 6d35ff25f001a43cbfecacc11e7d7591bb4808f9 
> 
> Diff: https://git.reviewboard.kde.org/r/127972/diff/
> 
> 
> Testing
> ---
> 
> Builds fine with flex 2.6.0 and bison 3.0.4; `make test` passes too.
> 
> 
> Thanks,
> 
> Pino Toscano
> 
>

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

Re: Review Request 127972: Always update the Predicate parser from y/l sources

[Pino Toscano]

> On May 20, 2016, 8:39 a.m., René J.V. Bertin wrote:
> > Looks good but I'll try to do a test-build during one of my next "lost 
> > moments".
> > 
> > Is there a minimum required version for either of the parser/generators?

> Is there a minimum required version for either of the parser/generators?

Not that I know of.


- Pino


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


On May 20, 2016, 6:39 a.m., Pino Toscano wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127972/
> ---
> 
> (Updated May 20, 2016, 6:39 a.m.)
> 
> 
> Review request for KDE Software on Mac OS X, KDE Frameworks, kdewin, and 
> Lukáš Tinkl.
> 
> 
> Repository: solid
> 
> 
> Description
> ---
> 
> Turn Flex and Bison into required build dependencies, and use them to always 
> regenerate at build time the Predicate parser. This ensures that the parser 
> does not rot, and there is no more need to rely on autogenerated sources 
> added statically among the others.
> 
> Second commit: remove old generated files of Predicate parser
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt 763e09cfeeebdc9e42b68e8ab6c9e29c54d3e741 
>   src/solid/CMakeLists.txt f2b43b27cb47531ed57b2eccafad8e67951b56b9 
>   src/solid/devices/CMakeLists.txt 9271ae1e36b67b112be54a6ff9c6fb76a8a0a824 
>   src/solid/devices/predicate_lexer.c 
> 3b5a0b90907baf1cd2631da4de650ec153d0f642 
>   src/solid/devices/predicate_parser.h 
> 68e25070d498f5a635489af51f4b772c5f374108 
>   src/solid/devices/predicate_parser.c 
> 6d35ff25f001a43cbfecacc11e7d7591bb4808f9 
> 
> Diff: https://git.reviewboard.kde.org/r/127972/diff/
> 
> 
> Testing
> ---
> 
> Builds fine with flex 2.6.0 and bison 3.0.4; `make test` passes too.
> 
> 
> Thanks,
> 
> Pino Toscano
> 
>

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

Re: Review Request 127972: Always update the Predicate parser from y/l sources

[Pino Toscano]

; >  make[1]: Leaving directory 
> > `/opt/local/var/macports/build/_opt_local_site-ports_kf5_KF5-Frameworks/kf5-solid/work/build'
> >  make: *** [all] Error 2
> >  make: Leaving directory 
> > `/opt/local/var/macports/build/_opt_local_site-ports_kf5_KF5-Frameworks/kf5-solid/work/build'
> > ```
> 
> René J.V. Bertin wrote:
> False alarm, builds fine with 5.22.0

Oh sorry, forgot to mention that you need to test the patch against git/master, 
since I fixed the grammar.

Did you also run `make test`?


- Pino


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


On May 20, 2016, 6:39 a.m., Pino Toscano wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127972/
> ---
> 
> (Updated May 20, 2016, 6:39 a.m.)
> 
> 
> Review request for KDE Software on Mac OS X, KDE Frameworks, kdewin, and 
> Lukáš Tinkl.
> 
> 
> Repository: solid
> 
> 
> Description
> ---
> 
> Turn Flex and Bison into required build dependencies, and use them to always 
> regenerate at build time the Predicate parser. This ensures that the parser 
> does not rot, and there is no more need to rely on autogenerated sources 
> added statically among the others.
> 
> Second commit: remove old generated files of Predicate parser
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt 763e09cfeeebdc9e42b68e8ab6c9e29c54d3e741 
>   src/solid/CMakeLists.txt f2b43b27cb47531ed57b2eccafad8e67951b56b9 
>   src/solid/devices/CMakeLists.txt 9271ae1e36b67b112be54a6ff9c6fb76a8a0a824 
>   src/solid/devices/predicate_lexer.c 
> 3b5a0b90907baf1cd2631da4de650ec153d0f642 
>   src/solid/devices/predicate_parser.h 
> 68e25070d498f5a635489af51f4b772c5f374108 
>   src/solid/devices/predicate_parser.c 
> 6d35ff25f001a43cbfecacc11e7d7591bb4808f9 
> 
> Diff: https://git.reviewboard.kde.org/r/127972/diff/
> 
> 
> Testing
> ---
> 
> Builds fine with flex 2.6.0 and bison 3.0.4; `make test` passes too.
> 
> 
> Thanks,
> 
> Pino Toscano
> 
>

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

Re: Review Request 127972: Always update the Predicate parser from y/l sources

[Pino Toscano]

> On May 20, 2016, 11:43 p.m., Nicolás Alvarez wrote:
> > Not only I approve of this change, but I also wish it was done over all 
> > other KDE software using flex/bison.

Yup, I will take care of othe cases like this one (they are just a few, luckly).


- Pino


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


On May 20, 2016, 6:39 a.m., Pino Toscano wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127972/
> ---
> 
> (Updated May 20, 2016, 6:39 a.m.)
> 
> 
> Review request for KDE Software on Mac OS X, KDE Frameworks, kdewin, and 
> Lukáš Tinkl.
> 
> 
> Repository: solid
> 
> 
> Description
> ---
> 
> Turn Flex and Bison into required build dependencies, and use them to always 
> regenerate at build time the Predicate parser. This ensures that the parser 
> does not rot, and there is no more need to rely on autogenerated sources 
> added statically among the others.
> 
> Second commit: remove old generated files of Predicate parser
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt 763e09cfeeebdc9e42b68e8ab6c9e29c54d3e741 
>   src/solid/CMakeLists.txt f2b43b27cb47531ed57b2eccafad8e67951b56b9 
>   src/solid/devices/CMakeLists.txt 9271ae1e36b67b112be54a6ff9c6fb76a8a0a824 
>   src/solid/devices/predicate_lexer.c 
> 3b5a0b90907baf1cd2631da4de650ec153d0f642 
>   src/solid/devices/predicate_parser.h 
> 68e25070d498f5a635489af51f4b772c5f374108 
>   src/solid/devices/predicate_parser.c 
> 6d35ff25f001a43cbfecacc11e7d7591bb4808f9 
> 
> Diff: https://git.reviewboard.kde.org/r/127972/diff/
> 
> 
> Testing
> ---
> 
> Builds fine with flex 2.6.0 and bison 3.0.4; `make test` passes too.
> 
> 
> Thanks,
> 
> Pino Toscano
> 
>

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

Re: Review Request 126610: kwidgetitemdelegate: properly cleanup widgets on index removal

[Pino Toscano]

> On March 28, 2016, 5:14 p.m., Stephen Kelly wrote:
> > Do you still have the sample application you made to test/verify this? I'd 
> > like to try it and it should probably be committed too.

No I don't :-/ I remember it was just removing indexes with associated widgets.


- Pino


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


On March 28, 2016, 1:41 p.m., Pino Toscano wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126610/
> ---
> 
> (Updated March 28, 2016, 1:41 p.m.)
> 
> 
> Review request for KDE Frameworks, David Edmundson and Stephen Kelly.
> 
> 
> Repository: kitemviews
> 
> 
> Description
> ---
> 
> When indexes are removed and their widgets deleted, the event filter on each 
> widget is not removed, leading to the "you should not delete widgets 
> manually"-alike warning.
> 
> Add an helper forgetAbout() function which performs all the actions done 
> per-widget before deleting each, additionally removing also the event filter.
> 
> 
> Diffs
> -
> 
>   src/kwidgetitemdelegate.cpp 779dc2a8a57148fb37f1f5a7194bc9656cb305a4 
>   src/kwidgetitemdelegatepool.cpp e916dddad8be56bb803e241da43d8cbe7a171ec3 
>   src/kwidgetitemdelegatepool_p.h 401fe193b0954d6c7c721503d4657b7f08e9fd2e 
> 
> Diff: https://git.reviewboard.kde.org/r/126610/diff/
> 
> 
> Testing
> ---
> 
> A sample application with widgets for items in the model, removing indexes: 
> no more warning at removal time.
> 
> 
> Thanks,
> 
> Pino Toscano
> 
>

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

Re: Review Request 127972: Always update the Predicate parser from y/l sources

[Pino Toscano]

> On May 20, 2016, 11:21 p.m., René J.V. Bertin wrote:
> > I've done some testing with Solid 5.20.0 .The patch applies cleanly, but 
> > I'm getting the error below. I'd write that down to using the older Solid 
> > version if it weren't for that fact I'm quite sure I've seen this kind of 
> > error before:
> > 
> > ```
> >  [ 30%] [BISON][SolidParser] Building parser with bison 3.0.4
> >  cd 
> > /opt/local/var/macports/build/_opt_local_site-ports_kf5_KF5-Frameworks/kf5-solid/work/solid-5.20.0/src/solid
> >  && /opt/local/bin/bison -p Solid -d -b predicate_parser -d -o 
> > /opt/local/var/macports/build/_opt_local_site-ports_kf5_KF5-Frameworks/kf5-solid/work/build/src/solid/predicate_parser.c
> >  devices/predicate_parser.y
> >  ...
> >  make[2]: Entering directory 
> > `/opt/local/var/macports/build/_opt_local_site-ports_kf5_KF5-Frameworks/kf5-solid/work/build'
> >  [ 31%] Building C object 
> > src/solid/CMakeFiles/KF5Solid_static.dir/predicate_parser.c.o
> >  cd 
> > /opt/local/var/macports/build/_opt_local_site-ports_kf5_KF5-Frameworks/kf5-solid/work/build/src/solid
> >  && 
> > /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang
> >   -DQT_CORE_LIB -DQT_DBUS_LIB -DQT_GUI_LIB -DQT_MAC_USE_COCOA 
> > -DQT_NO_CAST_TO_ASCII -DQT_NO_DEBUG -DQT_NO_SIGNALS_SLOTS_KEYWORDS 
> > -DQT_NO_URL_CAST_FROM_STRING -DQT_QSP_XDG_LIB -DQT_USE_FAST_OPERATOR_PLUS 
> > -DQT_USE_QSTRINGBUILDER -DQT_WIDGETS_LIB -DQT_XML_LIB -D_DARWIN_C_SOURCE 
> > -D_LARGEFILE64_SOURCE 
> > -I/opt/local/var/macports/build/_opt_local_site-ports_kf5_KF5-Frameworks/kf5-solid/work/build/src/solid
> >  
> > -I/opt/local/var/macports/build/_opt_local_site-ports_kf5_KF5-Frameworks/kf5-solid/work/solid-5.20.0/src/solid
> >  
> > -I/opt/local/var/macports/build/_opt_local_site-ports_kf5_KF5-Frameworks/kf5-solid/work/solid-5.20.0/src/solid/devices
> >  
> > -I/opt/local/var/macports/build/_opt_local_site-ports_kf5_KF5-Frameworks/kf5-solid/work/solid-5.20.0/src/solid/devices/frontend
> >  -I/opt/local/var/macports/build/_
 
opt_local_site-ports_kf5_KF5-Frameworks/kf5-solid/work/solid-5.20.0/src/solid/..
 
-I/opt/local/var/macports/build/_opt_local_site-ports_kf5_KF5-Frameworks/kf5-solid/work/build/src/solid/..
 -iframework /opt/local/libexec/qt5/Library/Frameworks -isystem 
/opt/local/libexec/qt5/Library/Frameworks/QtCore.framework/Headers -isystem 
/opt/local/share/qt5/mkspecs/macx-clang -isystem 
/opt/local/libexec/qt5/Library/Frameworks/QtQspXDG.framework/Headers -isystem 
/opt/local/libexec/qt5/Library/Frameworks/QtDBus.framework/Headers -isystem 
/opt/local/libexec/qt5/Library/Frameworks/QtXml.framework/Headers -isystem 
/opt/local/libexec/qt5/Library/Frameworks/QtWidgets.framework/Headers -isystem 
/opt/local/libexec/qt5/Library/Frameworks/QtGui.framework/Headers -isystem 
/System/Library/Frameworks/OpenGL.framework/Headers  -O3 -march=native -g 
-DNDEBUG -DQT_USE_EXTSTANDARDPATHS -DQT_EXTSTANDARDPATHS_XDG_DEFAULT=true  
-std=iso9899:1990 -fno-common -Wall -Wextra -Wcast-align -Wchar-subscripts 
-Wformat-secur
 ity -Wno-long-long -Wpointer-arith -Wundef -Wmissing-format-attribute 
-Wwrite-strings -Werror=implicit-function-declaration -arch x86_64 
-mmacosx-version-min=10.9 -fvisibility=hidden   -DSOLID_STATIC_DEFINE=1 -fPIC 
-o CMakeFiles/KF5Solid_static.dir/predicate_parser.c.o   -c 
/opt/local/var/macports/build/_opt_local_site-ports_kf5_KF5-Frameworks/kf5-solid/work/build/src/solid/predicate_parser.c
> >  
> > /opt/local/var/macports/build/_opt_local_site-ports_kf5_KF5-Frameworks/kf5-solid/work/build/src/solid/predicate_parser.c:1206:30:
> >  error: too few arguments to function call, expected 2, have 1
> >yychar = yylex (&yylval);
> > ~ ^
> >  devices/predicate_parser.y:13:1: note: 'Solidlex' declared here
> >  int Solidlex( YYSTYPE *yylval, yyscan_t scanner );
> >  ^
> >  devices/predicate_parser.y:96:17: error: too many arguments to function 
> > call, expected 0, have 1
> >  Solidparse( scanner );
> >  ~~  ^~~
> >  
> > /opt/local/var/macports/build/_opt_local_site-ports_kf5_KF5-Frameworks/kf5-solid/work/build/src/solid/predicate_parser.c:1036:1:
> >  note: 'Solidparse' declared here
> >  int
> >  ^
> >  2 errors generated.
> >  make[2]: *** 
> > [src/solid/CMakeFiles/KF5Solid_static.dir/predicate_parser.c.o] Error 1
> >  make[2]: Leaving directory 
> > `/opt/local/var/macports/build/_opt_local_site-ports_kf5_KF5-Frameworks/kf5-solid/work/build'
> >  make[1]: *** [src/solid/CMakeFiles/KF5Solid_static.dir/all] Error 2
> &

Review Request 127984: Always update the Trader parser from y/l sources

[Pino Toscano]

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

Review request for KDE Frameworks.


Repository: kservice


Description
---

Add Flex and Bison as required build dependencies, and use them to always 
regenerate at build time the Trader parser. This ensures that the parser does 
not rot, and there is no more need to rely on autogenerated sources added 
statically among the others.

Second commit: remove old generated files of Trader parser, and the helper 
regen.sh script.


Diffs
-

  CMakeLists.txt 7f9d4f03510c66f230e5c189e92d90a31beb2cf2 
  src/CMakeLists.txt cdcf88532cb8d7aba0cf7fbd24086d7f5905b6da 
  src/services/lex.c c811f67fc89a1a1ad0aef0de2feeba00ebd5d057 
  src/services/regen.sh c2af52b92767ca2dd0d1abe19eb6cf50642b5096 
  src/services/yacc.h 070ffa4bc895683b4a5b7502a031679c7d70c74a 
  src/services/yacc.c 675650a8d787c5d7e66ef5e2cf97bdeb65660f4b 

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


Testing
---

Builds fine with flex 2.6.0 and bison 3.0.4; `make test` passes too.


Thanks,

Pino Toscano

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

Review Request 123811: Use tcgetattr & tcsetattr if available

[Pino Toscano]

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

Review request for KDE Frameworks and Oswald Buddenhagen.


Repository: kpty


Description
---

Look for `tcgetattr` & `tcsetattr`, and use them if found before trying the own 
OS checks. They are specified by POSIX.1-2008, so they should be available on 
platforms implementing modern POSIX interfaces.

The rest of the fallback code is left as is (including manually selecting 
`tcgetattr` & `tcsetattr` on some OSes), to potentially avoid other platforms.


Diffs
-

  src/ConfigureChecks.cmake 35d7402567016c89c1fd947f44e29351aca4fe4a 
  src/config-pty.h.cmake e8dc262df0d1e6342e869fe17509b10a64baf300 
  src/kpty.cpp 145514cdee53dc81cf7453bd8d1514f173cb972e 

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


Testing
---

On Linux:

1. builds fine, and tests pass
2. manually set `HAVE_TCGETATTR` & `HAVE_TCSETATTR` to `FALSE`, the `ioctl` 
code is used as before, build is fine and so tests too 

Not tested on other OSes, but the second test above should make sure there's no 
build behaviour change if a platform does not have those functions.


Thanks,

Pino Toscano

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

Re: Review Request 123811: Use tcgetattr & tcsetattr if available

[Pino Toscano]

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

(Updated Mag. 16, 2015, 1:33 p.m.)


Review request for KDE Frameworks and Oswald Buddenhagen.


Changes
---

Fold code for platforms using tcgetattr & tcsetattr to the new block.


Repository: kpty


Description (updated)
---

Look for `tcgetattr` & `tcsetattr`, and use them if found before trying the own 
OS checks. They are specified by POSIX.1-2008, so they should be available on 
platforms implementing modern POSIX interfaces.

The rest of the fallback code is left as is for platforms not previously using 
`tcgetattr` & `tcsetattr`.


Diffs (updated)
-

  src/ConfigureChecks.cmake 35d7402567016c89c1fd947f44e29351aca4fe4a 
  src/config-pty.h.cmake e8dc262df0d1e6342e869fe17509b10a64baf300 
  src/kpty.cpp 145514cdee53dc81cf7453bd8d1514f173cb972e 

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


Testing
---

On Linux:

1. builds fine, and tests pass
2. manually set `HAVE_TCGETATTR` & `HAVE_TCSETATTR` to `FALSE`, the `ioctl` 
code is used as before, build is fine and so tests too 

Not tested on other OSes, but the second test above should make sure there's no 
build behaviour change if a platform does not have those functions.


Thanks,

Pino Toscano

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

Re: Review Request 123811: Use tcgetattr & tcsetattr if available

[Pino Toscano]

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

(Updated May 17, 2015, 8:47 a.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks and Oswald Buddenhagen.


Changes
---

Submitted with commit 35ea45b588db9afcbd796576833ac338c6b4b8e8 by Pino Toscano 
to branch master.


Repository: kpty


Description
---

Look for `tcgetattr` & `tcsetattr`, and use them if found before trying the own 
OS checks. They are specified by POSIX.1-2008, so they should be available on 
platforms implementing modern POSIX interfaces.

The rest of the fallback code is left as is for platforms not previously using 
`tcgetattr` & `tcsetattr`.


Diffs
-

  src/ConfigureChecks.cmake 35d7402567016c89c1fd947f44e29351aca4fe4a 
  src/config-pty.h.cmake e8dc262df0d1e6342e869fe17509b10a64baf300 
  src/kpty.cpp 145514cdee53dc81cf7453bd8d1514f173cb972e 

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


Testing
---

On Linux:

1. builds fine, and tests pass
2. manually set `HAVE_TCGETATTR` & `HAVE_TCSETATTR` to `FALSE`, the `ioctl` 
code is used as before, build is fine and so tests too 

Not tested on other OSes, but the second test above should make sure there's no 
build behaviour change if a platform does not have those functions.


Thanks,

Pino Toscano

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

Re: Review Request 127833: KWallet: More Coverity fixes, and include Qt headers for endianness check.

[Pino Toscano]

> On May 23, 2016, 11:20 a.m., David Faure wrote:
> > Please note that:
> > * the Q_BYTE_ORDER change was reverted, since it made the wallet storage 
> > incompatible with previous releases. This needs further analysis in order 
> > to improve the code while retaining compat.
> > * kwallet is definitely usable with a home install, I'm doing that right 
> > now. I do have the distro KF5 packages installed though, so maybe the 
> > polkit stuff got installed at the right place into the system as well. 
> > Anyhow it means it should be fixable with a one-time copy operation as root.
> 
> Michael Pyne wrote:
> That reminds me, I made an autotest for the Blowfish cipher that would 
> hopefully catch such errors in the future. I've committed the autotests now 
> (they pass, hopefully they don't break CI!).
> 
> With that autotest I think that Allen Winter's last Q_BYTE_ORDER change 
> had actually been correct (to enable the relevant code sections when 
> Q_BYTE_ORDER == Q_LITTLE_ENDIAN), but his first commit (to #include 
> qglobal.h) would have generated incorrect Wallets which would then not match 
> with KWallet when it was fixed.
> 
> Either way someone's systems are being broken here, because the current 
> code unilaterally enables bit-shuffling always no matter if the CPU is 
> big-endian or little-endian. But I don't have a big-endian machine to test on 
> to make sure a fix would (or would not) work.

This indeed shows that the current situation (kwallet 5.23) is broken on big 
endian platforms; looking at the failures we got when it was uploaded to Debian 
few days ago, 
https://buildd.debian.org/status/logs.php?pkg=kwallet-kf5&ver=5.23.0-1&suite=sid,
 we've got failures (related to this) on mips, powerpc, and hppa (s390x is not 
built yet, but it will fail there too) -- all of them are big endian.

I can help in testing patches fixing the `blowfishtest` unit test on those 
platforms.

> the current code unilaterally enables bit-shuffling always no matter if the 
> CPU is big-endian or little-endian.

A simple fix could then be use QtGlobal again, but invert the byte order 
conditions like:
-#if Q_BYTE_ORDER == Q_BIG_ENDIAN
+#if Q_BYTE_ORDER == Q_LITTLE_ENDIAN
this would preserve the compatibility with little endian platforms, although 
breaking wallets on big endian machines.


- Pino


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


On May 8, 2016, 12:10 a.m., Michael Pyne wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127833/
> ---
> 
> (Updated May 8, 2016, 12:10 a.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kwallet
> 
> 
> Description
> ---
> 
> This is a collection of minor fixes:
> 
> An uninit variable usage was noted by Coverity (CID 1289177) for a CBC crypto 
> function, though it only happens for encryption lengths that would not be hit 
> in practice. I troubleshot this in December but forgot to make a RR, but IIRC 
> the lengths that would cause problems are 7 bytes or less -- but it's still 
> better to fix.
> 
> The other Coverity fix is to avoid a needless dup(2) of an opened socket 
> since it's immediately turned into a FILE* object anyways (CID 1353007). This 
> avoids a minor resource leak of a file descriptor.
> 
> Finally, some of the ciphers use Qt checks for endianness, and need to 
> actually include the header that does this instead of relying on other parts 
> of the code incidentally pulling in the needed #includes.
> 
> 
> Diffs
> -
> 
>   src/runtime/kwalletd/backend/cbc.cc 4c13466 
>   src/runtime/kwalletd/main.cpp 90c60d8 
> 
> Diff: https://git.reviewboard.kde.org/r/127833/diff/
> 
> 
> Testing
> ---
> 
> Everything still compiles -- I'm limited in my ability to test since I'm 
> still using KDE4's KWallet (as the KF5 stuff seems to require polkit to 
> actually work, which isn't possible with a homedir install like mine).
> 
> 
> Thanks,
> 
> Michael Pyne
> 
>

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

Re: Review Request 127833: KWallet: More Coverity fixes, and include Qt headers for endianness check.

[Pino Toscano]

> On May 23, 2016, 11:20 a.m., David Faure wrote:
> > Please note that:
> > * the Q_BYTE_ORDER change was reverted, since it made the wallet storage 
> > incompatible with previous releases. This needs further analysis in order 
> > to improve the code while retaining compat.
> > * kwallet is definitely usable with a home install, I'm doing that right 
> > now. I do have the distro KF5 packages installed though, so maybe the 
> > polkit stuff got installed at the right place into the system as well. 
> > Anyhow it means it should be fixable with a one-time copy operation as root.
> 
> Michael Pyne wrote:
> That reminds me, I made an autotest for the Blowfish cipher that would 
> hopefully catch such errors in the future. I've committed the autotests now 
> (they pass, hopefully they don't break CI!).
> 
> With that autotest I think that Allen Winter's last Q_BYTE_ORDER change 
> had actually been correct (to enable the relevant code sections when 
> Q_BYTE_ORDER == Q_LITTLE_ENDIAN), but his first commit (to #include 
> qglobal.h) would have generated incorrect Wallets which would then not match 
> with KWallet when it was fixed.
> 
> Either way someone's systems are being broken here, because the current 
> code unilaterally enables bit-shuffling always no matter if the CPU is 
> big-endian or little-endian. But I don't have a big-endian machine to test on 
> to make sure a fix would (or would not) work.
> 
> Pino Toscano wrote:
> This indeed shows that the current situation (kwallet 5.23) is broken on 
> big endian platforms; looking at the failures we got when it was uploaded to 
> Debian few days ago, 
> https://buildd.debian.org/status/logs.php?pkg=kwallet-kf5&ver=5.23.0-1&suite=sid,
>  we've got failures (related to this) on mips, powerpc, and hppa (s390x is 
> not built yet, but it will fail there too) -- all of them are big endian.
> 
> I can help in testing patches fixing the `blowfishtest` unit test on 
> those platforms.
> 
> > the current code unilaterally enables bit-shuffling always no matter if 
> the CPU is big-endian or little-endian.
> 
> A simple fix could then be use QtGlobal again, but invert the byte order 
> conditions like:
> -#if Q_BYTE_ORDER == Q_BIG_ENDIAN
> +#if Q_BYTE_ORDER == Q_LITTLE_ENDIAN
> this would preserve the compatibility with little endian platforms, 
> although breaking wallets on big endian machines.
> 
> Michael Pyne wrote:
> If you can test a patch, I'd recommend Allen Winter's last fix attempt 
> before the whole thing was reverted, at commit 
> 87e774825b779ba846315a8b2ffe6479dd9f9814. This should implement your 
> suggestion already, it was only reverted since there continued to be 
> complaints of brokenness.
> 
> It is my feeling that his patch was correct, and that the problems noted 
> were by users who had recreated a wallet during the 34-hour window where the 
> cipher was broken for all (due to commit 
> b3a95ba0540e01a9bb10db53fc449cc49ce9a9e8 activating Q_BYTE_ORDER checks in 
> reverse). If wallets generated in this window are broken then they would 
> always be treated as corrupt by correct code. But note that the current 
> autotest only checks the cipher for compliance against Blowfish's own test 
> vectors, not whether wallets generated using it can be reopened later.

Apologies for the late reply.

> If you can test a patch, I'd recommend Allen Winter's last fix attempt before 
> the whole thing was reverted, at commit 
> 87e774825b779ba846315a8b2ffe6479dd9f9814.

If I backport b3a95ba0540e01a9bb10db53fc449cc49ce9a9e8 and 
87e774825b779ba846315a8b2ffe6479dd9f9814 for 
`src/runtime/kwalletd/backend/blowfish.cc`, then indeed the test passes on ppc. 
Should I go for this, and commit these bits?

I wonder also about the sha1 code (`src/runtime/kwalletd/backend/sha1.cc`): 
ideally this should go away and be replaced by `QCryptographicHash`, but it's 
an exported class... So let's migrate all its usages to `QCryptographicHash` 
and deprecate it?


- Pino


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


On May 8, 2016, 12:10 a.m., Michael Pyne wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127833/
> ---
> 
> (Updated May 8, 2016, 12:10 a.m.)
> 
> 
> Review request for KDE Frameworks.
> 
>

Re: Review Request 127972: Always update the Predicate parser from y/l sources

[Pino Toscano]

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

(Updated July 10, 2016, 5:18 a.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Software on Mac OS X, KDE Frameworks, kdewin, and Lukáš 
Tinkl.


Changes
---

Submitted with commit 31daba692c020943453150a626ed3cf19562676b by Pino Toscano 
to branch master.


Repository: solid


Description
---

Turn Flex and Bison into required build dependencies, and use them to always 
regenerate at build time the Predicate parser. This ensures that the parser 
does not rot, and there is no more need to rely on autogenerated sources added 
statically among the others.

Second commit: remove old generated files of Predicate parser


Diffs
-

  CMakeLists.txt 763e09cfeeebdc9e42b68e8ab6c9e29c54d3e741 
  src/solid/CMakeLists.txt f2b43b27cb47531ed57b2eccafad8e67951b56b9 
  src/solid/devices/CMakeLists.txt 9271ae1e36b67b112be54a6ff9c6fb76a8a0a824 
  src/solid/devices/predicate_lexer.c 3b5a0b90907baf1cd2631da4de650ec153d0f642 
  src/solid/devices/predicate_parser.h 68e25070d498f5a635489af51f4b772c5f374108 
  src/solid/devices/predicate_parser.c 6d35ff25f001a43cbfecacc11e7d7591bb4808f9 

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


Testing
---

Builds fine with flex 2.6.0 and bison 3.0.4; `make test` passes too.


Thanks,

Pino Toscano

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

Re: Review Request 127984: Always update the Trader parser from y/l sources

[Pino Toscano]

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

(Updated July 9, 2016, 10:27 p.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks.


Changes
---

Submitted with commit 0cf33ffbf88777a86635948ed03917253342569c by Pino Toscano 
to branch master.


Repository: kservice


Description
---

Add Flex and Bison as required build dependencies, and use them to always 
regenerate at build time the Trader parser. This ensures that the parser does 
not rot, and there is no more need to rely on autogenerated sources added 
statically among the others.

Second commit: remove old generated files of Trader parser, and the helper 
regen.sh script.


Diffs
-

  CMakeLists.txt 7f9d4f03510c66f230e5c189e92d90a31beb2cf2 
  src/CMakeLists.txt cdcf88532cb8d7aba0cf7fbd24086d7f5905b6da 
  src/services/lex.c c811f67fc89a1a1ad0aef0de2feeba00ebd5d057 
  src/services/regen.sh c2af52b92767ca2dd0d1abe19eb6cf50642b5096 
  src/services/yacc.h 070ffa4bc895683b4a5b7502a031679c7d70c74a 
  src/services/yacc.c 675650a8d787c5d7e66ef5e2cf97bdeb65660f4b 

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


Testing
---

Builds fine with flex 2.6.0 and bison 3.0.4; `make test` passes too.


Thanks,

Pino Toscano

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

Re: Review Request 127106: applications.menu: remove references to unused categories

[Pino Toscano]

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



Ping

- Pino Toscano


On March 28, 2016, 1:41 p.m., Pino Toscano wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127106/
> ---
> 
> (Updated March 28, 2016, 1:41 p.m.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Repository: kservice
> 
> 
> Description
> ---
> 
> Stop handling few categories long deprecated (replaced by standard 
> categories):
> 
> - `X-KDE-KDevelopIDE` (with associated, and not existing, 
> `kf5-development-kdevelop.directory`) -- KDevelop does not use it anymore 
> since long
> - `X-KDE-Edu-Language` -- replaced by Languages
> - `X-KDE-KidsGame` -- replaced by KidsGame
> 
> Nothing on kde.org references them, and neither anything currently packaged 
> in Debian.
> 
> 
> Diffs
> -
> 
>   src/applications.menu f2d52538c4ed6dc39d04a10e7c072cb060b3fd2b 
> 
> Diff: https://git.reviewboard.kde.org/r/127106/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Pino Toscano
> 
>

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

Re: Review Request 127106: applications.menu: remove references to unused categories

[Pino Toscano]

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

(Updated July 10, 2016, 9:38 a.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks and David Faure.


Changes
---

Submitted with commit c1ca6fe180d171e365024e1f51f003095ae962fc by Pino Toscano 
to branch master.


Repository: kservice


Description
---

Stop handling few categories long deprecated (replaced by standard categories):

- `X-KDE-KDevelopIDE` (with associated, and not existing, 
`kf5-development-kdevelop.directory`) -- KDevelop does not use it anymore since 
long
- `X-KDE-Edu-Language` -- replaced by Languages
- `X-KDE-KidsGame` -- replaced by KidsGame

Nothing on kde.org references them, and neither anything currently packaged in 
Debian.


Diffs
-

  src/applications.menu f2d52538c4ed6dc39d04a10e7c072cb060b3fd2b 

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


Testing
---


Thanks,

Pino Toscano

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

D17816: Support for xattrs on kio copy/move

[Pino Toscano]

pino added a comment.


  In D17816#627921 , @cochise wrote:
  
  > In D17816#627918 , @bruns wrote:
  >
  > > Not true in general ... please add back, and **check** if the destination 
FS supports XAttrs. If not, print a warning and SKIP
  >
  >
  > Not true in general. But test the copies to /tmp, and on most distros today 
it's a tmpfs, that don't support xattr. But checking support is a good idea.
  
  
  
  
$ grep -i xattr /boot/config-$(uname -r) | grep -i tmp
CONFIG_TMPFS_XATTR=y
  
  xattrs on the Linux tmpfs have been supported for many years, almost a decade.

REPOSITORY
  R241 KIO

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

To: cochise, dfaure, chinmoyr, bruns, #frameworks, tmarshall
Cc: usta, scheirle, anthonyfieroni, tmarshall, arrowd, cfeck, bruns, phidrho, 
dhaumann, funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, 
spoorun, nicolasfella, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh

D28324: [Inotify] Remove dead/duplicate code

[Pino Toscano]

pino added inline comments.

INLINE COMMENTS

> bruns wrote in kinotify.cpp:350
> First check is here

... but only also if wd is < 0

REPOSITORY
  R293 Baloo

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

To: bruns, #baloo, ngraham
Cc: pino, kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, cblack, 
fbampaloukas, GB_2, domson, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D28324: [Inotify] Remove dead/duplicate code

[Pino Toscano]

pino added inline comments.

INLINE COMMENTS

> bruns wrote in kinotify.cpp:350
> man inotify
> 
> > IN_Q_OVERFLOW
> >  Event queue overflowed (wd is -1 for this event).

then add an assert for this case, referencing the documentation?

REPOSITORY
  R293 Baloo

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

To: bruns, #baloo, ngraham
Cc: pino, kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, cblack, 
fbampaloukas, GB_2, domson, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D28355: Introduce function ecm_install_configured_file

[Pino Toscano]

pino added a comment.


  Can you please adapt it so `_template` can be an absolute path?

REPOSITORY
  R240 Extra CMake Modules

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

To: davidedmundson
Cc: pino, kde-frameworks-devel, kde-buildsystem, LeGast00n, cblack, GB_2, 
bencreasy, michaelh, ngraham, bruns

D28324: [Inotify] Remove dead/duplicate code

[Pino Toscano]

pino added inline comments.

INLINE COMMENTS

> bruns wrote in kinotify.cpp:350
> asserts are IMHO pointless, nobody enables them, but it clutters the code. 
> Anyone touching the code should and has to read the man pages etc anyway.

no, an assert is sort of "program by contract": you make it clear that this 
situation ought to not happen, and it if does, at least some people may notice 
that

> Anyone touching the code should and has to read the man pages etc anyway.

and an explicit mention/comment in the code never killed anyone, nor made the 
code "cluttered"; also what is valid today might change in the future, so 
writing down why a check/assert was added make sure that people reading the 
changed documentation know why it made sense when it was added

REPOSITORY
  R293 Baloo

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

To: bruns, #baloo, ngraham
Cc: pino, kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, cblack, 
fbampaloukas, GB_2, domson, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D28355: Introduce function ecm_install_configured_file

[Pino Toscano]

pino added a comment.


  Nice one! I cannot test right now though, I might do it over the weekend (do 
not hold on me though).
  
  I took the liberty of doing some formatting changes to the header of the new 
file, what do you think?
  
#.rst:
# ECMConfiguredInstall
# 
#
# Take as ``.cmake.in`` file, runs ``configure_file`` and installs it in the
# given location.
#
# ::
#
#   ecm_install_configured_file( )
#
# Example usage:
#
# .. code-block:: cmake
#
#   ecm_install_configured_file(foo.txt.cmake.in 
${KDE_INSTALL_FULL_DATADIR})
#
# This will install the file as ``foo.txt`` with any cmake replacements made
# into the data directory.
#
# Since 5.69.0.


#=
# Copyright 2020  David Edmundson 
  
  Also, not sure whether you need a .rst file in the docs/module/ directory.

INLINE COMMENTS

> ECMConfiguredInstall.cmake:46-48
> +# strip .cmake.in from the end
> +# if that isn't there, we continue as-is
> +string(REGEX REPLACE ".cmake.in$"  "" _name ${_name})

considering we are documenting the input file as `.cmake.in`, should we enforce 
this here and ignore any file not ending like that?

REPOSITORY
  R240 Extra CMake Modules

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

To: davidedmundson
Cc: pino, kde-frameworks-devel, kde-buildsystem, LeGast00n, cblack, GB_2, 
bencreasy, michaelh, ngraham, bruns

D28355: Introduce function ecm_install_configured_file

[Pino Toscano]

pino added inline comments.

INLINE COMMENTS

> davidedmundson wrote in ECMConfiguredInstall.cmake:46-48
> Maybe.
> 
> My rationale for not forcing a suffix is sometimes we do this configure dance 
> for .desktop files and there we have to be a bit careful with scripty.
> 
> But generally it's neater and easier to use when a suffix is used. Currently 
> Plasma is all over the place with what suffix to have, so I picked one at 
> random.

No problem either way. My idea is that if a precise suffix is required, then 
setting it //before// this function is merged is better, otherwise doing it 
once it is already in use means breaking potential (mis)users.

REPOSITORY
  R240 Extra CMake Modules

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

To: davidedmundson
Cc: pino, kde-frameworks-devel, kde-buildsystem, LeGast00n, cblack, GB_2, 
bencreasy, michaelh, ngraham, bruns

D28397: Replace Vokoscreen with VokoscreenNG

[Pino Toscano]

pino added a comment.


  please do //not// change translated keys (eg `Name[lang]`) in desktop-alike 
files when you change the English string: we have a system in place to handle 
translations

REPOSITORY
  R304 KNewStuff

BRANCH
  update-vokoscreen-url (branched from master)

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

To: harogaston, #knewstuff, ngraham, leinir
Cc: pino, IlyaBizyaev, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
ngraham, bruns

D28532: Introduce more user-visible error reporting for installations

[Pino Toscano]

pino added a comment.


  Please fix the i18n() calls, as the values of placeholders are passed as 
parameter to it instead of using .arg():
  
i18n("foo %1").arg(foo)  // WRONG
i18n("foo %1", foo)  // correct

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, #plasma, ngraham, #frameworks
Cc: pino, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D28673: [PackageUrlInterceptor] Make QRegularExpression static

[Pino Toscano]

pino added a comment.


  Does it really need to be a regular expression, though? A simple string 
search & replace should do the same thing.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: broulik, #plasma
Cc: pino, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D28755: Breeze Icons cannot be built from read-only source location

[Pino Toscano]

pino added a comment.


  Or maybe the other way round:
  
  - add an optional parameter to the script to specify the source directory, 
defaulting to "."
  - run the script in the build directory, passing the source directory as 
parameter

REPOSITORY
  R266 Breeze Icons

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

To: marten, #breeze
Cc: pino, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D28760: KSettings::Dialog: avoid duplicate entries due cascading $XDG_DATA_DIRS

[Pino Toscano]

pino added a comment.


  Why not instead use a `QMap` to collect the files? This way you wouldn't need the double 
QStandardPaths lookup.

REPOSITORY
  R295 KCMUtils

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

To: dfaure, apol, broulik, davidedmundson, kossebau
Cc: pino, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28755: Breeze Icons cannot be built from read-only source location

[Pino Toscano]

pino added a comment.


  Fixed with def21bf6c691a40bef233efd898fcb7ff57d55bb 
, 
thanks for the notice.

REPOSITORY
  R266 Breeze Icons

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

To: marten, #breeze, ngraham
Cc: bcooksley, ngraham, pino, kde-frameworks-devel, LeGast00n, cblack, 
michaelh, bruns

D29063: Fix testpackage-appstream: XDG_DATA_DIRS needs to be explicitly extended

[Pino Toscano]

pino added a comment.


  (Commented in the wrong place)
  
  Shouldn't the test completely ignore the system location, to avoid 
interferences from the system installation?

REPOSITORY
  R290 KPackage

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

To: kossebau, #frameworks, mart, apol, sitter, bcooksley
Cc: pino, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29254: [RenameDialog] Add an arrow indicating direction from src to dest

[Pino Toscano]

pino added inline comments.

INLINE COMMENTS

> renamedialog.cpp:299
> +// direction from src to dest
> +const QPixmap pix = 
> QIcon::fromTheme(QStringLiteral("go-next")).pixmap(d->m_srcPreview->height());
> +srcToDestArrow->setPixmap(pix);

this is not right-to-left aware; please use the layout direction of the widget 
to use "go-next" or "go-previous"

REPOSITORY
  R241 KIO

BRANCH
  l-srt-to-dest (branched from master)

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

To: ahmadsamir, #frameworks, dfaure, meven, ngraham
Cc: pino, ngraham, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns

D29299: Make KI18N_INSTALL() not rely on only LOCALE_INSTALL_DIR

[Pino Toscano]

pino added a comment.


  The problem is that ki18n_install() is rarely used manually, and generally it 
is appended by the release scripts to the top-level CMakeLists.txt file that 
goes into the tarballs.
  This means that either the majority of the packages will not make use of 
this, or a mass-bump of the ki18n dependency (which I'd rather not do).
  
  I personally prefer Heiko's approach (D29136 
), followed by making ECM required to use 
ki18n.

REPOSITORY
  R249 KI18n

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

To: kossebau, ilic, heikobecker, #frameworks, aacid, ltoscano
Cc: pino, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29299: Make KI18N_INSTALL() not rely on only LOCALE_INSTALL_DIR

[Pino Toscano]

pino added a comment.


  > The problem is that ki18n_install() is rarely used manually
  
  OK, at least from LXR search it seems a bit more than that: KF packages using 
ki18n, some extragear stuff, and few Plasma bits.
  This means that KF packages can switch to that immediately (because of the 
strict dependency requirement), however most of the rest won't soon (like 
Marble, which will stay unfixed until an ECM requirement bump).

REPOSITORY
  R249 KI18n

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

To: kossebau, ilic, heikobecker, #frameworks, aacid, ltoscano
Cc: pino, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29299: Make KI18N_INSTALL() not rely on only LOCALE_INSTALL_DIR

[Pino Toscano]

pino added a comment.


  > The patch should not require existing users to adapt
  
  Yes, that's also what I wrote earlier.
  
  Also, your patch basically includes D29136 
 in the case of no DESTINATION parameter 
specified, hence my suggestion is:
  
  - edit D29136  to do the fallback using 
the same logic introduced here: this way marble is already fixed with no other 
changes, and ki18n_install will work also with KDE_INSTALL_DIRS_NO_DEPRECATED 
(e.g. for release-service packages)
  - have this to add the DESTINATION parameter, so packages can opt-in to use 
it if they can/want

REPOSITORY
  R249 KI18n

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

To: kossebau, ilic, heikobecker, #frameworks, aacid, ltoscano
Cc: pino, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29299: Make KI18N_INSTALL() not rely on only LOCALE_INSTALL_DIR

[Pino Toscano]

pino added a comment.


  In D29299#660466 , @kossebau wrote:
  
  > In D29299#660465 , @pino wrote:
  >
  > > Also, your patch basically includes D29136 
 in the case of no DESTINATION parameter 
specified, hence my suggestion is:
  > >
  > > - edit D29136  to do the fallback 
using the same logic introduced here: this way marble is already fixed with no 
other changes, and ki18n_install will work also with 
KDE_INSTALL_DIRS_NO_DEPRECATED (e.g. for release-service packages)
  > > - have this to add the DESTINATION parameter, so packages can opt-in to 
use it if they can/want
  >
  >
  > Not exactly sure what you mean? Do you want two separate commits/reviews, 
one per issue?
  
  
  Yes, and we have them already: D29136  
(to reopen) and this (which would need to rebased on the former).

REPOSITORY
  R249 KI18n

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

To: kossebau, ilic, heikobecker, #frameworks, aacid, ltoscano
Cc: pino, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29299: Make KI18N_INSTALL() not rely on only LOCALE_INSTALL_DIR

[Pino Toscano]

pino added a comment.


  In D29299#660490 , @kossebau wrote:
  
  > D29136  in the current version though 
changes behaviour by favouring KDE_INSTALL_LOCALEDIR over LOCALE_INSTALL_DIR. 
Which at least in theory might somewhere in some distant galaxy break things ;)
  
  
  Which is correct, since KDE_INSTALL_LOCALEDIR is set only by ECM (and 
hopefully nothing else), so it should be safe to use as preferred location.
  
  Do you have any specific concern, instead of "might" and "distant galaxy"? 
Because if not, I'd rather just pull D29136 
 in, and call it a day.

REPOSITORY
  R249 KI18n

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

To: kossebau, ilic, heikobecker, #frameworks, aacid, ltoscano
Cc: pino, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29299: Make KI18N_INSTALL() not rely on only LOCALE_INSTALL_DIR

[Pino Toscano]

pino added a comment.


  In D29299#660505 , @kossebau wrote:
  
  > Because people do strange things, and I prefer to rather not break their 
card house unless necessary.
  
  
  Again, in which cases? The only way it might change the path is: use ki18n 
AND build with cmake AND use the ki18n_install macro AND define a 
KDE_INSTALL_LOCALEDIR variable yourself. Too many factors to make it even 
something to even bother thinking about TBH.
  
  Let's just use D29136 , so it will use 
the non-deprecated variable no matter whether KDE_INSTALL_DIRS_NO_DEPRECATED is 
set.

REPOSITORY
  R249 KI18n

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

To: kossebau, ilic, heikobecker, #frameworks, aacid, ltoscano
Cc: pino, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29299: Make KI18N_INSTALL() not rely on only LOCALE_INSTALL_DIR

[Pino Toscano]

pino added a comment.


  In D29299#660614 , @kossebau wrote:
  
  > In D29299#660519 , @pino wrote:
  >
  > > In D29299#660505 , @kossebau 
wrote:
  > >
  > > > Because people do strange things, and I prefer to rather not break 
their card house unless necessary.
  > >
  > >
  > > Again, in which cases? The only way it might change the path is: use 
ki18n AND build with cmake AND use the ki18n_install macro AND define a 
KDE_INSTALL_LOCALEDIR variable yourself. Too many factors to make it even 
something to even bother thinking about TBH.
  >
  >
  > Mileages difer :)
  >
  > One does not need to define KDE_INSTALL_LOCALEDIR oneself. One only needs 
to use find_package(KF5Plasma) or find_package(KF5Package), because some 
subrepo also does a plasma applet for Plasma integration or have someone 
cluelessly copied cmake code together because ENOTIMEFORBUILDSYSTEM and copying 
from the best=KDE and now-it-builds-so-do-not-touch-again.
  
  
  In this case it will be generated automatically, and it will be the same as 
LOCALE_INSTALL_DIR.
  
  > I have seen quite some cmake code, otherwise I would not be so passive here 
in the change (and yes, there is a world outside kde repos) :)  And when you 
recommneded ki18n outside of kde spheres over the sad Qt i18n stuff, you do not 
want to see things falling apart, You yourself might not bother, but I do, 
because I have been hit myself too often in my life by people carelessly 
changing default things in minor releases, because 
worksformecanotimaginesomethingstrangewhybother.
  
  Let me clarify: I **do** care about software outside kde.org, you are 
definitely not the only one that does that. What I wrote above about "not 
bothering" refers to //that// specific situation I listed, not to everything 
else.
  I'm also one of the people that says "beware of compatibility"... when it 
makes sense. In this case, I do not see anything to keep compatibility with, 
and the only situation is already a broken case on its own.
  
  Again: beside what I said earlier, do you see any **actual** potential other 
scenarios, nor situation where what I proposed would not be safe?
  
  > Why stop people caring for doing things more safely? :)
  
  I am not, please do not put thoughts in my mind that were not mine, thanks :)

REPOSITORY
  R249 KI18n

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

To: kossebau, ilic, heikobecker, #frameworks, aacid, ltoscano
Cc: pino, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29254: [RenameDialog] Add an arrow indicating direction from src to dest

[Pino Toscano]

pino added inline comments.

INLINE COMMENTS

> ahmadsamir wrote in renamedialog.cpp:299
> @pino, right; dolphin isn't rtl-aware (or if it is, I couldn't find out how 
> to switch it to rtl). But I agree the code here should account for rtl anyway.
> 
> @ngraham: I think we should stick to the icon naming spec[1], so that it 
> works with themes other than breeze/oxygen; so it has to be go-previous.
> 
> [1] 
> https://specifications.freedesktop.org/icon-naming-spec/icon-naming-spec-latest.html

> (or if it is, I couldn't find out how to switch it to rtl).



  $ dolphin --reverse

it is provided directly by QApplication, it works also in Qt-only applications

REPOSITORY
  R241 KIO

BRANCH
  l-srt-to-dest (branched from master)

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

To: ahmadsamir, #frameworks, dfaure, meven, ngraham
Cc: hpereiradacosta, pino, ngraham, kde-frameworks-devel, LeGast00n, cblack, 
michaelh, bruns

D29381: Thumbnail text: use libmagic to detect encoding

[Pino Toscano]

pino added a comment.


  why not KEncodingProber from the KCodecs framework, like also the commented 
bits hint about?

REPOSITORY
  R320 KIO Extras

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

To: meven, #frameworks, sitter, ngraham
Cc: pino, kde-frameworks-devel, kfm-devel, azyx, nikolaik, pberestov, iasensio, 
aprcela, fprice, LeGast00n, cblack, fbampaloukas, alexde, Codezela, feverfew, 
meven, michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, 
emmanuelp, rdieter, mikesomov

D29805: Thumbnail djvu: avoid to exit(1) when it should not

[Pino Toscano]

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


  This change is definitely not correct.
  
  > A child process exited with 1 as result but it wasn't an error case.
  
  it is an error case: the execv*() family [1] of functions replaces the 
execution with the requested process, and //never return// in a successful 
execution. "successful execution" means that the process was started correctly, 
no matter its resulting exit code. Anything after execv*() will be processed 
only if it fails, and one of the most common issues is that the requested 
executable does not exists.
  
  In addition to this, your case merely changes the exit() code, still calling 
exit() (which appears in the backtraces of the two bugs, and most probably one 
can be marked as duplicate of the other). The issue here is that fork() 
//clones// the calling process (in this case kdeinit), including the atexit 
handlers: apparently a mesa atexit handler is run... The real fix in this case 
is to use _exit() [2], which is a more direct way to exit the current process 
without running the atexit handlers.
  
  Furthermore, this code looks like a C snippet to invoke a process and reading 
its stdout retrofitted as ThumbCreator plugin... sadly C is not an easy 
language, and the process APIs are complex and very prone to mistakes. The 
ideal solution would be to invoke ddjvu using QProcess, and read its output as 
it comes; it would also have the (small) advantage of making this thumbnailer 
plugin available on any platform, Windows included (in case anyone cases, that 
is).
  
  Let me sum up what I think is the approach to make this ThumbCreator work 
properly:
  
  - to fix the issue in the stable branch:
- switch exit() wth _exit(), using the same exit code (otherwise `ok` won't 
be set as false after the waitpid() call in the parent)
- before the pipe creation, check for the ddjvu executable existance using 
QStandardPath::findExecutable, returning false quickly
  - to fix the issue in the long term:
- rewrite this to call ddjvu using QProcess
  
  [1] https://pubs.opengroup.org/onlinepubs/9699919799/functions/exec.html
  [2] https://pubs.opengroup.org/onlinepubs/9699919799/functions/_Exit.html

REPOSITORY
  R320 KIO Extras

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

To: meven, #frameworks, broulik, ngraham, pino
Cc: pino, kde-frameworks-devel, kfm-devel, waitquietly, azyx, nikolaik, 
pberestov, iasensio, aprcela, fprice, LeGast00n, cblack, fbampaloukas, alexde, 
Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, rdieter, mikesomov

D29805: Thumbnail djvu: avoid to exit(1) when it should not

[Pino Toscano]

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


  Please update the text of the revision and the commit message, as they don't 
make much sense. Please mention that it fixes a crash in case ddjvu is not 
installed, by avoiding executing it, and using _exit() to not run atexit 
handlers in case execvp() fails.
  
  Also, please respect the indentation of the code: 2 spaces, and no brackets 
for a single instruction in a block.

REPOSITORY
  R320 KIO Extras

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

To: meven, #frameworks, broulik, ngraham, pino
Cc: pino, kde-frameworks-devel, kfm-devel, waitquietly, azyx, nikolaik, 
pberestov, iasensio, aprcela, fprice, LeGast00n, cblack, fbampaloukas, alexde, 
Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, rdieter, mikesomov

D28673: [PackageUrlInterceptor] Make QRegularExpression static

[Pino Toscano]

pino added a comment.


  In D28673#672845 , @broulik wrote:
  
  > so, can the regexp be replaced or does it need to stay?
  
  
  Sure: look for `/ui/` in path, and if it is there, join what's before it + 
prefix + what's after it.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: broulik, #plasma
Cc: bruns, pino, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham

D29805: Thumbnail djvu: Avoid a crash when djvu is not installed

[Pino Toscano]

pino added a comment.


  please target `release/20.04` for this fix, thanks

INLINE COMMENTS

> djvucreator.cpp:52-54
> +  if (QStandardPaths::findExecutable(QStringLiteral("ddjvu")).isEmpty()) {
> + return false;
> +  }

extra brackets

REPOSITORY
  R320 KIO Extras

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

To: meven, #frameworks, broulik, ngraham, pino
Cc: pino, kde-frameworks-devel, kfm-devel, waitquietly, azyx, nikolaik, 
pberestov, iasensio, aprcela, fprice, LeGast00n, cblack, fbampaloukas, alexde, 
Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, rdieter, mikesomov

D29805: Thumbnail djvu: Avoid a crash when djvu is not installed

[Pino Toscano]

pino added inline comments.

INLINE COMMENTS

> meven wrote in djvucreator.cpp:52-54
> I'd like to use instead the Framework coding style to improve homogenizing 
> coding styles.
> https://community.kde.org/Policies/Frameworks_Coding_Style#Braces

this code does not follow that style, so please keep new changes coherent with 
the existing style

REPOSITORY
  R320 KIO Extras

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

To: meven, #frameworks, broulik, ngraham, pino
Cc: pino, kde-frameworks-devel, kfm-devel, waitquietly, azyx, nikolaik, 
pberestov, iasensio, aprcela, fprice, LeGast00n, cblack, fbampaloukas, alexde, 
Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, rdieter, mikesomov

D29805: Thumbnail djvu: Avoid a crash when djvu is not installed

[Pino Toscano]

pino added a comment.


  > FIXED-IN: 20.08
  
  still for 20.08...

REPOSITORY
  R320 KIO Extras

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

To: meven, #frameworks, broulik, ngraham, pino
Cc: pino, kde-frameworks-devel, kfm-devel, waitquietly, azyx, nikolaik, 
pberestov, iasensio, aprcela, fprice, LeGast00n, cblack, fbampaloukas, alexde, 
Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, rdieter, mikesomov

D29805: Thumbnail djvu: Avoid a crash when djvu is not installed

[Pino Toscano]

pino added a comment.


  In D29805#674205 , @meven wrote:
  
  > In D29805#674204 , @pino wrote:
  >
  > > > FIXED-IN: 20.08
  > >
  > > still for 20.08...
  >
  >
  > Yes kio-extra is released with KDE  Applications
  
  
  Yes, I know. Asked to land this fix instead to `release/20.04`, and thus 
change the commit message accordingly.

REPOSITORY
  R320 KIO Extras

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

To: meven, #frameworks, broulik, ngraham, pino
Cc: pino, kde-frameworks-devel, kfm-devel, waitquietly, azyx, nikolaik, 
pberestov, iasensio, aprcela, fprice, LeGast00n, cblack, fbampaloukas, alexde, 
Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, rdieter, mikesomov

D29805: Thumbnail djvu: Avoid a crash when djvu is not installed

[Pino Toscano]

pino added a comment.


  In D29805#674218 , @meven wrote:
  
  > In D29805#674206 , @pino wrote:
  >
  > > In D29805#674205 , @meven 
wrote:
  > >
  > > > In D29805#674204 , @pino 
wrote:
  > > >
  > > > > > FIXED-IN: 20.08
  > > > >
  > > > > still for 20.08...
  > > >
  > > >
  > > > Yes kio-extra is released with KDE  Applications
  > >
  > >
  > > Yes, I know. Asked to land this fix instead to `release/20.04`, and thus 
change the commit message accordingly.
  >
  >
  > Please be explicit when you comment, no one could deduce what you meant.
  
  
  I wrote it **two** times to land this on the stable branch: the first time 
when I explained why the initial patch was not correct and what the problem 
actually was (with hints on short term and long term fixes), and earlier today 
when I wrote:
  
  In D29805#674185 , @pino wrote:
  
  > please target `release/20.04` for this fix, thanks
  
  
  There is no need to "deduce" anything, just read what I wrote, thanks.
  
  > If this is ready approve and add a comment "land to 20.04".
  
  The commit message still says "20.08", so not yet.

REPOSITORY
  R320 KIO Extras

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

To: meven, #frameworks, broulik, ngraham, pino
Cc: pino, kde-frameworks-devel, kfm-devel, waitquietly, azyx, nikolaik, 
pberestov, iasensio, aprcela, fprice, LeGast00n, cblack, fbampaloukas, alexde, 
Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, rdieter, mikesomov

D27633: Port to KPluginLoader

[Pino Toscano]

pino added a comment.


  In D27633#619369 , @aacid wrote:
  
  > In D27633#619365 , @aacid wrote:
  >
  > > I think this broke 
https://build.kde.org/job/Applications/job/ktp-common-internals/job/kf5-qt5%20SUSEQt5.12/20/console
 guess ¿KAccountsDPlugin now requires parameters to the constructor and is thus 
not a valid Q_INTERFACE?
  > >
  > > @nicolasfella can you please look at it?
  >
  >
  > On top of that that's a BIC change, you can't do BIC changes on KF5 repos, 
so revert?
  
  
  Even if it is not a Framework, this is still a BIC change without an SONAME 
bump; @nicolasfella what about bumping `KACCOUNTS_SOVERSION` in the top-level 
CMakeLists.txt to `2` to reflect that?

REPOSITORY
  R155 KAccounts Integration

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

To: nicolasfella, bshah, leinir, #frameworks, apol
Cc: pino, aacid, lbeltrame

D29299: Make KI18N_INSTALL() not rely on only LOCALE_INSTALL_DIR

[Pino Toscano]

pino added a comment.


  In D29299#676439 , @dfaure wrote:
  
  > @pino Other than the fact that you think D29136 
 is "good enough", do you have any concrete 
objection to this version?
  
  
  I think I already wrote enough comments on this...
  
  I asked for actual **valid** use cases when using the new variables first 
would break, and I still got none. There is a limit to how much you can keep 
broken code working... assuming such broken code exists. I don't think there is 
any of this such situation, as `ki18n_install()` is basically used by KF 
sources that use ECM already, with marble being the only exception (and even 
that, marble won't break).
  
  Oh, and just to make it clear: none of my comments implies that I don't care 
about ECM, nor about any users of it, nor that I "like" to purposefully break 
cmake scripts.

REPOSITORY
  R249 KI18n

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

To: kossebau, ilic, heikobecker, #frameworks, aacid, ltoscano
Cc: dfaure, pino, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, 
bruns

D29299: Make KI18N_INSTALL() not rely on only LOCALE_INSTALL_DIR

[Pino Toscano]

pino added a comment.


  In D29299#676446 , @dfaure wrote:
  
  > In D29299#676445 , @pino wrote:
  >
  > > I asked for actual **valid** use cases when using the new variables first 
would break, and I still got none. There is a limit to how much you can keep 
broken code working... assuming such broken code exists. I don't think there is 
any of this such situation, as `ki18n_install()` is basically used by KF 
sources that use ECM already, with marble being the only exception (and even 
that, marble won't break).
  >
  >
  > As you know, there are KF5-based applications outside the realm of what we 
can see in LXR.
  >  One of the primary goals of KF5 is to be useable by other applications not 
written by the KDE community (I actually know quite a few).
  >  As such, it's not hard to imagine a cmake-based application that uses Qt 
and GNUInstallDirs [with qmake going away this will happen more and more], and 
one day it wants to use one of the frameworks. At that point, it shouldn't be 
forced to switch to ECMInstallDirs. Therefore I definitely see value in keeping 
the two things separate, as long as we keep making things easy for what is the 
most common case for us: using both.
  
  
  Sigh. I know this, I never, ever, ever, and let me say it again, **never**, 
forgot about this.
  
  > This is why I'm requesting that the integration with ECM is called 
integration and not "backwards compat fallback".
  > 
  > You say you don't want to support broken code. I agree. Would you agree 
that the situation I'm describing here is NOT broken code?
  
  Sure. But it is not what I referred to when I spoke about "broken code".
  
  >> Oh, and just to make it clear: none of my comments implies that I don't 
care about ECM, nor about any users of it, nor that I "like" to purposefully 
break cmake scripts.
  > 
  > I didn't say any of this, and you're replying to me here, not to Friedrich. 
I'm trying to bring this whole thing to a solution, so let's move aside all 
such accusations and concentrate on what might be helpful to resolve the 
technical issue.
  
  Yes, sorry, you are not Friedrich. OTOH, it would be nice to not be painted 
as "the one that wants to break things without second thoughts". Can we please 
agree on this? Because if not, there is no more point for me to keep contribute 
to a discussion.
  
  Again, I already proposed a technical solution. Let me quote it again:
  
  In D29299#660465 , @pino wrote:
  
  > > The patch should not require existing users to adapt
  >
  > Yes, that's also what I wrote earlier.
  >
  > Also, your patch basically includes D29136 
 in the case of no DESTINATION parameter 
specified, hence my suggestion is:
  >
  > - edit D29136  to do the fallback using 
the same logic introduced here: this way marble is already fixed with no other 
changes, and ki18n_install will work also with KDE_INSTALL_DIRS_NO_DEPRECATED 
(e.g. for release-service packages)
  > - have this to add the DESTINATION parameter, so packages can opt-in to use 
it if they can/want

REPOSITORY
  R249 KI18n

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

To: kossebau, ilic, heikobecker, #frameworks, aacid, ltoscano
Cc: dfaure, pino, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, 
bruns

D29299: Make KI18N_INSTALL() not rely on only LOCALE_INSTALL_DIR

[Pino Toscano]

pino added a comment.


  In D29299#676448 , @dfaure wrote:
  
  > When you wrote "ki18n_install() is basically used by KF sources that use 
ECM already" it seemed to me that this was looking at KDE community code only
  
  
  FWIW, I also checked https://sources.debian.org/ (which is the collection of 
sources in the Debian releases). I'm not aware of other services like this.
  
  In D29299#676448 , @dfaure wrote:
  
  > If you agree that being able to use ki18n without ECM is better, then 
indeed we all agree.
  
  
  If that wouldn't had been the case, it would have said to prefer D29136 
.
  
  >> Sure. But it is not what I referred to when I spoke about "broken code".
  > 
  > I have to apologize again, then, because I don't understand what is the 
"broken code" we're talking about then.
  
  "broken code" is when a cmake script (mis)uses undocumented/internal 
bits/variables not documented as such. Or for example when you redefine macros, 
internal variables, or stuff like that.
  
  >>> Also, your patch basically includes D29136 
 in the case of no DESTINATION parameter 
specified, hence my suggestion is:
  >>> 
  >>> - edit D29136  to do the fallback 
using the same logic introduced here: this way marble is already fixed with no 
other changes, and ki18n_install will work also with 
KDE_INSTALL_DIRS_NO_DEPRECATED (e.g. for release-service packages)
  > 
  > Would this be what is done in D29303 ? 
(I just learned about this third option...)
  
  Kinda, with the difference that it still prefers the deprecated variables. 
This is why I prefer D29136 .

REPOSITORY
  R249 KI18n

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

To: kossebau, ilic, heikobecker, #frameworks, aacid, ltoscano
Cc: dfaure, pino, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, 
bruns

D13549: Switch KIO::convertSize() to KFormat::formatByteSize()

[Pino Toscano]

pino created this revision.
pino added a reviewer: dfaure.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: kde-frameworks-devel.
pino requested review of this revision.

REVISION SUMMARY
  Make KIO::convertSize() use KFormat::formatByteSize(), instead of using
  an own copy of the same function. The handling of the user
  configuration of the default dialect is left as-is, and it should work
  fine since the two enums BinaryUnitDialect (the removed one, and the
  one in KFormat) are the same.

TEST PLAN
  Builds fine, no changes to the result of KIO::convertSize().

REPOSITORY
  R241 KIO

BRANCH
  kformat (branched from master)

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

AFFECTED FILES
  src/core/global.cpp

To: pino, dfaure
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13585: Use LIB_SUFFIX for lmdb libdir rather than hardcode lib

[Pino Toscano]

pino added a comment.


  Or just drop the two `HINTS` (for `find_path` and `find_library`) altogether, 
as cmake already looks in standard include/library paths.

REPOSITORY
  R293 Baloo

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

To: asturmlechner, #baloo
Cc: pino, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, 
spoorun, ngraham, bruns, abrahams

D13585: Use LIB_SUFFIX for lmdb libdir rather than hardcode lib

[Pino Toscano]

pino added a comment.


  In D13585#279400 , @asturmlechner 
wrote:
  
  > The problem with that is cmake picking the wrong dir (first in order is 
`lib`, while it should be e.g. `lib64`).
  
  
  Sure, and that's why I suggested to drop the usage of `LMDB_DIR`. There is 
already `CMAKE_PREFIX_PATH` 
 to 
specify additional prefixes where to search for installed software, so 
`LMDB_DIR` is simply an ad-hoc version of it.
  In case this is the chosen way, make sure to port away from where `LMDB_DIR` 
is used: https://lxr.kde.org/search?_filestring=&_string=LMDB_DIR

REPOSITORY
  R293 Baloo

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

To: asturmlechner, #baloo
Cc: pino, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, 
spoorun, ngraham, bruns, abrahams

D13549: Switch KIO::convertSize() to KFormat::formatByteSize()

[Pino Toscano]

pino added a comment.


  Polite ping.

REPOSITORY
  R241 KIO

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

To: pino, dfaure
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13549: Switch KIO::convertSize() to KFormat::formatByteSize()

[Pino Toscano]

pino added inline comments.

INLINE COMMENTS

> aacid wrote in global.cpp:44
> Am i reading the code wrong or are we doing the readEntry twice? (i know the 
> old code did the same)
> 
> Do you understand why?

> Am i reading the code wrong or are we doing the readEntry twice? (i know the 
> old code did the same)

Apparently so.

> Do you understand why?

No, that is why I chose to not touch at all, keeping the behaviour unchanged 
(it is not related to this patch, anyway).

REPOSITORY
  R241 KIO

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

To: pino, dfaure
Cc: aacid, kde-frameworks-devel, michaelh, ngraham, bruns

D13549: Switch KIO::convertSize() to KFormat::formatByteSize()

[Pino Toscano]

pino updated this revision to Diff 3.
pino marked an inline comment as done.
pino added a comment.


  Fix precision passed to formatByteSize().

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13549?vs=36179&id=3

BRANCH
  kformat (branched from master)

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

AFFECTED FILES
  src/core/global.cpp

To: pino, dfaure
Cc: aacid, kde-frameworks-devel, michaelh, ngraham, bruns

D13549: Switch KIO::convertSize() to KFormat::formatByteSize()

[Pino Toscano]

pino marked an inline comment as done.
pino added inline comments.

INLINE COMMENTS

> aacid wrote in global.cpp:58
> Should this -1 be 1? Looking at the kformat code it'll use that unless unit 
> is 0 so the old code would be using 1 and the new one -1

Indeed, 1 is the right value, and `KFormat` does the right thing when the unit 
is byte.

REPOSITORY
  R241 KIO

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

To: pino, dfaure
Cc: aacid, kde-frameworks-devel, michaelh, ngraham, bruns

D13940: Add syntax highlighting support for Stan

[Pino Toscano]

pino added a comment.


  It looks like your forgot to include the actual `highlight.stan` file for the 
test suite, can you please amend this commit to include it?

REPOSITORY
  R216 Syntax Highlighting

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

To: jeffreyarnold, #framework_syntax_highlighting
Cc: pino, kde-frameworks-devel, michaelh, ngraham, bruns

D13549: Switch KIO::convertSize() to KFormat::formatByteSize()

[Pino Toscano]

pino closed this revision.

REPOSITORY
  R241 KIO

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

To: pino, dfaure, aacid
Cc: aacid, kde-frameworks-devel, michaelh, ngraham, bruns

D14111: Install MIME type definition for text/x-rst ourselves for now

[Pino Toscano]

pino added a comment.


  NACK.
  Make sure that the request for shared-mime-info 
(https://bugs.freedesktop.org/show_bug.cgi?id=107227) is accepted, and that is 
enough. There is enough stuff in `kde5.xml` already, even shipped in 
shared-mime-info, so adding more is not a good idea.

REPOSITORY
  R244 KCoreAddons

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

To: kossebau, dfaure, pino
Cc: fabianr, kde-frameworks-devel, michaelh, ngraham, bruns

D14111: Install MIME type definition for text/x-rst ourselves for now

[Pino Toscano]

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


  In D14111#291934 , @kossebau wrote:
  
  > In D14111#291933 , @pino wrote:
  >
  > > NACK.
  > >  Make sure that the request for shared-mime-info 
(https://bugs.freedesktop.org/show_bug.cgi?id=107227) is accepted, and that is 
enough.
  >
  >
  > Even if the request is accepted, it will take some time until it makes it 
to the users/developers, given smo 1.10 was tagged/released only some days ago, 
and as smo release cycle seems to be half a year or longer. Which in developer 
life is ages, especially looking at the web rivals, which deliver each months 
to everyone :/
  
  
  If that is problematic enough, talk with hadess (Bastien Nocera) about that.
  
  >> There is enough stuff in `kde5.xml` already, even shipped in 
shared-mime-info, so adding more is not a good idea.
  > 
  > Sure that needs some clean-up. I already started a patch to split this up 
and make things depend on the found smo version, so just the stuff is installed 
which is really needed.
  
  I thought about a similar approach in the past, but
  
  - unless you really maintain the amount of files, and properly tie each to a 
version of s-m-i, it becomes a mess to maintain
  - once you update s-m-i, you are back to a potential conflict (since the new 
s-m-i version may carry a new mimetype shipped in `kde5.xml`)
  - related to the point above: s-m-i is really a //runtime// dependency, so 
version checks at //build// time are not exactly good ideas...
  
  > Perhaps you can reconsider your take on this one once that has been 
uploaded and reviewed :)
  
  Not really: even if you implement what you mention above, the duplication is 
still there.
  
  Just to expand a bit more: when we switched to s-m-i during the kde4 
development (more than 12 years ago), I took the task of migrating our 
mimetypes to the "new" format. Call it "mistakes of youth", "limitations of the 
toolchain at that point", etc, the result was a single `kde.xml` carrying all 
the mimetype definitions that kdelibs had, and that s-m-i had not. Some of the 
mimetypes were generic enough to be added to s-m-i, so they were included (see 
various commits with myself as author), and some were not (see the various "smi 
rejected" comments floating around). To overcome the lack of these mimetypes in 
s-m-i, they were added in `kde.xml`, because of the reasons you mention above. 
Soon though I realized it was not a good idea, since a) neither of the 
mimetypes were critical enough to warrant duplicates b) basically nobody else 
than @dfaure me maintained `kde.xml`. See for example the recent 
e5f09f84a945af4edf4f346aed409acf29905414 
, 
which is one of the biggest cleanups after so many years. Or 
a7be6bab411d7f1fe2555ab5adc0465ca0cfc5d9 
, 
i.e. synchronize a local copy of a s-m-i mimetype ...
  
  So yes, I understand your reasons, but because of all the history involved I 
really do not want to add more stuff to `kde5.xml` that is really material for 
s-m-i.

REPOSITORY
  R244 KCoreAddons

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

To: kossebau, dfaure, pino
Cc: fabianr, kde-frameworks-devel, michaelh, ngraham, bruns

D14134: KFormat: fix typo in SI prefix name enum

[Pino Toscano]

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


  It must be fixed in `src/lib/util/kformatprivate.cpp` too.

REPOSITORY
  R244 KCoreAddons

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

To: bruns, #frameworks, astippich, pino
Cc: pino, kde-frameworks-devel, michaelh, ngraham, bruns

D14135: KFormat: fix typo in SI prefix name enum

[Pino Toscano]

pino added a comment.


  Duplicate of D14134: KFormat: fix typo in SI prefix name enum 
? If so, please abandon this.

REPOSITORY
  R244 KCoreAddons

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

To: bruns, #frameworks, astippich
Cc: pino, kde-frameworks-devel, michaelh, ngraham, bruns

D14135: KFormat: fix typo in SI prefix name enum

[Pino Toscano]

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

REPOSITORY
  R244 KCoreAddons

BRANCH
  prefix_typo

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

To: bruns, #frameworks, astippich, pino
Cc: pino, kde-frameworks-devel, michaelh, ngraham, bruns

D14360: Remove custom icon selection for trash

[Pino Toscano]

pino added a comment.


  TBH creating a widget without adding it to a layout does not make much sense 
to me -- in the past I saw those situations as "floating" widgets usually 
anchored at the top-left corner of the parent widget.
  The best way here is to just not create `m_iconButton` at all, like I 
suggested in D14378: Remove custom icon selection for trash 
 too.

REPOSITORY
  R241 KIO

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

To: shubham, ngraham, broulik, #dolphin, #frameworks
Cc: pino, kde-frameworks-devel, michaelh, ngraham, bruns

D14360: Remove custom icon selection for trash

[Pino Toscano]

pino added a comment.


  In D14360#298208 , @shubham wrote:
  
  > pino: I don't why m_iconButton isn't needed(because it is being used by 
other entries also ), am I right?
  
  
  I don't understandwhat you mean, can you please rephrase it?
  
  The point is: if the icon must not be edited, then even creating the 
`KiconButton` for it is not useful, because it's an unused widget. Even more, 
Creating it and making it hidden still uses the resources needed to create it, 
and load the icon for it. So... just do not create it, taking care of handling 
in the dialog the case when `m_iconButton` is null.

REPOSITORY
  R241 KIO

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

To: shubham, ngraham, broulik, #dolphin, #frameworks
Cc: pino, kde-frameworks-devel, michaelh, ngraham, bruns

D14360: Remove custom icon selection for trash

[Pino Toscano]

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


  It is a good start, but not enough:
  
  - `m_iconButton` must be set to null in the constructor (ideally at the 
beginning, in the initializer list), otherwise it will be uninitialized memory
  - `KFilePlaceEditDialog::icon()` will crash now, so its usage of 
`m_iconButton` must be guarded; either you return the icon passed to the 
constructor (which thus you need to save), or adjust callers to ask whether the 
icon could be changed, and only in that case get the icon

INLINE COMMENTS

> kfileplaceeditdialog.cpp:123
> +
> +// Draw the icon button for the entry only if it's *NOT* TRASH
> +if (url.scheme() != QLatin1String("trash")) {

No need to SCREAM in comments :)

REPOSITORY
  R241 KIO

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

To: shubham, ngraham, broulik, #dolphin, #frameworks, dfaure, pino
Cc: pino, kde-frameworks-devel, michaelh, ngraham, bruns

D14360: Remove custom icon selection for trash

[Pino Toscano]

pino requested changes to this revision.
pino added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> kfileplaceeditdialog.cpp:66
> +// Get icon except for trash
> +if (url.scheme() != QLatin1String("trash")) {
> +icon = dialog->icon();

This duplicates the logic that is used in the constructor below. A better 
option might be to create an helper class method to get whether the icon can be 
edited, basically checking for `m_iconButton`.

> kfileplaceeditdialog.cpp:123
>  
>  whatsThisText = i18n("This is the icon that will appear in the 
> Places panel."
>   "Click on the button to select a different 
> icon.");

This variable assignment can be moved inside the `if` as well, since this 
"what's this" text is used only for the icon button & its label.

> kfileplaceeditdialog.cpp:132
> +m_iconButton->setIconType(KIconLoader::NoGroup, KIconLoader::Place);
> +
> +if (icon.isEmpty()) {

Extra empty line.

> kfileplaceeditdialog.cpp:138
> +}
> +
> +m_iconButton->setWhatsThis(whatsThisText);

Extra empty line.

REPOSITORY
  R241 KIO

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

To: shubham, ngraham, broulik, #dolphin, #frameworks, dfaure, pino
Cc: pino, kde-frameworks-devel, michaelh, ngraham, bruns

D14360: Remove custom icon selection for trash

[Pino Toscano]

pino added a comment.


  In D14360#299439 , @shubham wrote:
  
  > pino, I understood your idea of having a helper class function, but the 
condition for editing the icon will remain the same ( based on its scheme), 
what can be the other condition?
  
  
  For example if in the future you exclude another scheme from icon editing. 
Generally speaking, there is a logic here (`url.scheme() != 
QLatin1String("trash")`), and usually duplicating it even across the very same 
source file is not a good idea.

REPOSITORY
  R241 KIO

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

To: shubham, ngraham, broulik, #dolphin, #frameworks, dfaure, pino
Cc: pino, kde-frameworks-devel, michaelh, ngraham, bruns

D14360: Remove custom icon selection for trash

[Pino Toscano]

pino added a comment.


  In D14360#299441 , @shubham wrote:
  
  > indirectly you mean to transfer all the logic to helper function
  
  
  No, I suggested something like:
  
bool KFilePlaceEditDialog::canEditIcon() const
{
  return m_iconButton;
}
  
  using it in `KFilePlaceEditDialog::getInformation()`.

REPOSITORY
  R241 KIO

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

To: shubham, ngraham, broulik, #dolphin, #frameworks, dfaure, pino
Cc: pino, kde-frameworks-devel, michaelh, ngraham, bruns

D14360: Remove custom icon selection for trash

[Pino Toscano]

pino added a comment.


  In D14360#299443 , @shubham wrote:
  
  > pino: suppose this function is created(canEditIcon()),then this function 
must  also be called when we are creating the m_iconButton at line 123 so as to 
generalize it's creation ( as you had said to take into account other entries 
also if they are also to be excluded).
  
  
  The function I suggested shows to the external users of 
`KFilePlaceEditDialog` (which is only the static 
`KFilePlaceEditDialog::getInformation()`) whether the icon could be edited or 
not.

REPOSITORY
  R241 KIO

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

To: shubham, ngraham, broulik, #dolphin, #frameworks, dfaure, pino
Cc: pino, kde-frameworks-devel, michaelh, ngraham, bruns

D14360: Remove custom icon selection for trash

[Pino Toscano]

pino added a comment.


  In D14360#301270 , @cfeck wrote:
  
  > I still would like to understand why it needs to be public API, instead of 
just an `@internal` method.
  
  
  This is not a public API, since `KFilePlaceEditDialog` is a private class 
which is not even exported.

INLINE COMMENTS

> kfileplaceeditdialog.cpp:65
>  label   = dialog->label();
> -icon= dialog->icon();
> +if (dialog->m_iconButton) {
> +icon = dialog->icon();

This is even worse than the method. Please switch back to the method, since it 
is the better solution,

REPOSITORY
  R241 KIO

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

To: shubham, ngraham, broulik, #dolphin, #frameworks, pino
Cc: cfeck, pino, kde-frameworks-devel, michaelh, ngraham, bruns

D14360: Remove custom icon selection for trash

[Pino Toscano]

pino requested changes to this revision.
This revision now requires changes to proceed.

REPOSITORY
  R241 KIO

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

To: shubham, ngraham, broulik, #dolphin, #frameworks, pino
Cc: cfeck, pino, kde-frameworks-devel, michaelh, ngraham, bruns

D14360: Remove custom icon selection for trash

[Pino Toscano]

pino added a comment.


  In D14360#301497 , @cfeck wrote:
  
  > If it's not public, it doesn't need a `@since` tag.
  
  
  No idea, it was not me to suggest that.
  
  > Why isn't the header called *_p.h ...?
  
  No idea either.

REPOSITORY
  R241 KIO

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

To: shubham, ngraham, broulik, #dolphin, #frameworks, pino
Cc: cfeck, pino, kde-frameworks-devel, michaelh, ngraham, bruns

D14504: Save few string allocations

[Pino Toscano]

pino added a comment.


  TBH I had the idea of making `splitLocale()` public, since it was a public 
API in `KLocale`. I still see a couple of users of it, and in the past I 
remember people working around the lack of `splitLocale()` by doing the same 
job on their own, when porting away from kdelibs4support.
  
  With that in mind, IMHO using `QStringRef` for a public API still seems odd 
to me, especially that if you really care about any part split, then most 
probably you will pass to other APIs that use `QString` (sort of nullifying the 
gain from using `QStringRef`).
  
  I saw too many cases in kdelibs 4.x applications using patterns like:
  
QString language, dummy;
KLocale::splitLocale(locale, language, dummy, dumy, dummy);
  
  Because of that, a better API would be to pass `QString*` for the split 
parts, so you can decide what actually you want as results -- e.g.:
  
QString language;
KLocalizedString::splitLocale(locale, &language, nullptr, nullptr, nullptr);

REPOSITORY
  R249 KI18n

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

To: apol, #frameworks, ilic
Cc: pino, kde-frameworks-devel, michaelh, ngraham, bruns

D14449: Modify device usage information

[Pino Toscano]

pino added inline comments.

INLINE COMMENTS

> kpropertiesdialog.cpp:1189
> +grid->addWidget(l, curRow, 2);
> +l->setText(i18nc("Device usage information","%1 used , %2 free", 
> KIO::convertSize(size - free), KIO::convertSize(free)));
> +

Also, style for i18n strings: there must be no space before comma.

REPOSITORY
  R241 KIO

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

To: shubham, ngraham, #frameworks, rkflx
Cc: pino, rkflx, dhaumann, kde-frameworks-devel, michaelh, ngraham, bruns

D14757: Warn user before copy operation if available space is not enough

[Pino Toscano]

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


  Also, considering this is in a job in `KIOCore` (i.e. non-gui library), I 
suspect that using a message box directly is the wrong way to do it. Most 
probably you need to use the UI delegate of the job for this.

INLINE COMMENTS

> CMakeLists.txt:146
>   KF5::Service
> + KF5::WidgetsAddons
>   Qt5::Network

This is a private dependency, so it must go to the `PRIVATE` section of 
`target_link_libraries`.

> copyjob.cpp:66
>  
> -
>  #include 

Unneeded change.

> copyjob.cpp:890
> +if (m_totalSize > m_freeSpace) {
> +KMessageBox::warningYesNo(nullptr, i18n("You do not have 
> sufficient space available. Do you still want to continue?"));
> +}

The return value of `KMessageBox::warningYesNo` is ignored, so this will 
continue regardless of the user choice.
Also, the message itself is not informative enough: what are the space needed, 
and the space available? And which directory for?

REPOSITORY
  R241 KIO

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

To: shubham, broulik, ngraham, pino
Cc: pino, kde-frameworks-devel, michaelh, ngraham, bruns

D14236: Add some improvements to kate-syntax-highlighter for use in scripting

[Pino Toscano]

pino added inline comments.

INLINE COMMENTS

> htmlhighlighter.h:42-43
>  void highlightFile(const QString &fileName);
> +void highlightFile(const QString &fileName, const QString &title);
> +void highlightFile(QFile &file, const QString &title);
>  

Instead of both the variants, what about a single one with QIODevice, e.g.:

  void highlightData(QIODevice *dev, const QString &title = QString());

this way you can highlight also an in-memory buffer, for example.

(For the same reason, a `setOutputDevice(QIODevice *dev)` would be nice too, 
but it's out of scope for this patch.)

REPOSITORY
  R216 Syntax Highlighting

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

To: xciml, #framework_syntax_highlighting, vkrause
Cc: pino, kde-frameworks-devel, kwrite-devel, #framework_syntax_highlighting, 
michaelh, genethomas, kevinapavew, ngraham, bruns, demsking, cullmann, vkrause, 
sars, dhaumann

D14757: Warn user before copy operation if available space is not enough

[Pino Toscano]

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


  Hint: please make sure you build **and** test the next versions of your 
patches, otherwise it is just a waste of everybody's time.

INLINE COMMENTS

> copyjob.cpp:891
> +if (m_totalSize > m_freeSpace) {
> +SimpleJobPrivate *sjp;
> +int msgRes;

Uninitialized pointer, this will crash two lines later...
Also, this is the base class of the private class used for this job, and this 
function is part of that class already; so why aren't you just invoking it?

> copyjob.cpp:893
> +int msgRes;
> +msgRes = 
> sjp->requestMessageBox(JobUiDelegateExtension::WarningYesNo,
> +QString("Warning!"),

The order of the parameters does not match at all the actual parameters of the 
function.
Also, all the text/caption strings **must** be translated.

> copyjob.cpp:899
> + "Do you still want to 
> continue?",
> +  m_dest.path(),
> +  
> KIO::convertSize(m_totalSize),

Assuming `m_dest` is a local file, then `toLocalFile()` is the right function 
to call (`path()` will give a different result on Windows).

> copyjob.cpp:909
> +
> +if (msgRes == KMessageBox::Yes) {
> +goto yes;

The return value is `JobUiDelegateExtension::MessageBoxType`, not 
`KMessageBox::ButtonCode`.

REPOSITORY
  R241 KIO

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

To: shubham, pino, dfaure
Cc: dfaure, pino, kde-frameworks-devel, michaelh, ngraham, bruns

D14859: Don't try to call transaction documentUrl with a 0 id

[Pino Toscano]

pino requested changes to this revision.
pino added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> main.cpp:140
>  fid = Baloo::devIdAndInodeToId(devId, inode);
> -url = QFile::decodeName(tr.documentUrl(fid));
> +if (fid) {
> +url = QFile::decodeName(tr.documentUrl(fid));

maybe `fid > 0` for clarity

> main.cpp:143
> +} else {
> +stream << i18n("No index information for") << url << endl;
> +continue;

string puzzle, please use placeholders for i18n()

REPOSITORY
  R293 Baloo

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

To: jtamate, #frameworks, pino
Cc: pino, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, 
spoorun, ngraham, bruns, abrahams

D17991: Refactor the way device backends are built and registered

[Pino Toscano]

pino added a comment.


  ping?

REPOSITORY
  R245 Solid

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

To: pino
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D18770: [KStatusNotifierItem] use fallback sizes when none is available

[Pino Toscano]

pino created this revision.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
pino requested review of this revision.

REVISION SUMMARY
  When converting a QIcon to a vector of images to D-Bus, the list of
  available sizes is used to extract the pixmaps of the icon.  In case
  the engine of the icon advertizes no sizes, then no pixmaps are sent:
  this is the case of the SVG icon engine shipped with QtSvg, so creating
  a QIcon from an SVG file means nothing is sent for it.
  
  As solution, use a list of few well-known sizes in case no size is
  available: this way there is the possibility to have some pixmaps for
  that icon.

TEST PLAN
  - create a KStatusNotifierItem
  - create a QIcon from a SVG file
  - set that QIcon as pixmap for the KStatusNotifierItem, e.g. using 
setIconByPixmap
  
  Without this change, no icon is shown in the Plasma tray.

REPOSITORY
  R289 KNotifications

BRANCH
  ksni-fallback-icons (branched from master)

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

AFFECTED FILES
  src/kstatusnotifieritem.cpp

To: pino
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D18883: Add PDF thumbnailer

[Pino Toscano]

pino added inline comments.

INLINE COMMENTS

> pdfcreator.cpp:45-46
> +{
> +Q_UNUSED(width);
> +Q_UNUSED(height);
> +

please honor the requested width and height

> pdfcreator.cpp:49
> +QScopedPointer document;
> +document.reset(Document::load(QFile::encodeName(path)));
> +

Document::load() takes a QString, so do not encode the filename (which will be 
encoded twice)

> pdfthumbnail.desktop:6
> +X-KDE-ServiceTypes=ThumbCreator
> +MimeType=application/pdf;application/x-dvi;application/postscript;
> +

definitely neither DVI nor PS...

REPOSITORY
  R320 KIO Extras

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

To: broulik, dfaure, aacid, jtamate
Cc: pino, ltoscano, kde-frameworks-devel, kfm-devel, alexde, feverfew, 
michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, 
emmanuelp, mikesomov

D18883: Add PDF thumbnailer

[Pino Toscano]

pino added inline comments.

INLINE COMMENTS

> pdfcreator.cpp:23
> +
> +#include 
> +#include 

the QFile include is no more needed now

> broulik wrote in pdfcreator.cpp:45-46
> I can't. The `renderToImage` can only be told a resolution or part of the 
> page to render, to render downscaled into a certain box.
> 
> The `ThumbnailJob` downscales the image when it exceeds the requested size, 
> so doing any manual downscaling (other than already getting the correct size 
> which we can't) here is superfluous.

Sure you can: see what okular does, for example, as it requests pixmaps of 
precise sizes.

REPOSITORY
  R320 KIO Extras

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

To: broulik, dfaure, aacid, jtamate
Cc: pino, ltoscano, kde-frameworks-devel, kfm-devel, alexde, feverfew, 
michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, 
emmanuelp, mikesomov

D18882: [Image Thumbnailer] Support eps files

[Pino Toscano]

pino added a comment.


  OTOH this will not work if kimageformats is not installed, and the 
thumbnailer gives no hint about that.

REPOSITORY
  R320 KIO Extras

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

To: broulik, jtamate, dfaure, ngraham
Cc: pino, ngraham, kde-frameworks-devel, kfm-devel, alexde, feverfew, michaelh, 
spoorun, navarromorales, firef, andrebarros, bruns, emmanuelp, mikesomov

D18882: [Image Thumbnailer] Support eps files

[Pino Toscano]

pino added a comment.


  Then just ship a desktop file for that format in kimageformats:
  
  - if kio-extras is not installed, that desktop file will be unused (although 
just a couple of kilobytes on disk)
  - if ko-extras is installed, it will register PS as thumbnail format using 
imagethumbnail
  
  The same change can IMHO be done for all the formats currently in MimeType, 
and that provided by kimageformats.

REPOSITORY
  R320 KIO Extras

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

To: broulik, jtamate, dfaure, ngraham
Cc: pino, ngraham, kde-frameworks-devel, kfm-devel, alexde, feverfew, michaelh, 
spoorun, navarromorales, firef, andrebarros, bruns, emmanuelp, mikesomov