Re: RCC for icons - update: Re: Icons installed by apps

[Jaroslaw Staniek]

On 18 February 2016 at 18:25, Kåre Särs  wrote:

> On Thursday, February 18, 2016 02:51:58 PM René J.V. Bertin wrote:
> > On Thursday February 18 2016 13:55:18 Jaroslaw Staniek wrote:
> > >>> > since QIcon::fromTheme() apaprently isn't able to find app icons.
> >
> > Care to explain? QIcon::fromTheme() doesn't find anything "out of the
> box"
> > on OS X (and I presume on MS Windows), but that is only because no theme
> > search path is set on those platforms. When you add the standard XDG icon
> > repository to the icon search path on OS X, even "pure" Qt5 application
> > will start showing icons all over if you have an icon theme installed --
> > including in widgets that should not have icons according to the HIG.
> Also
> > on OS X, fromTheme() will only return the application icon (as in the
> icon
> > shown in the Finder) if the current theme defines that same icon for the
> > calling application, and the theme search path is set of course. In all
> > other cases it will not, because the application icon is not defined
> > through a theme on OS X (nor is it on MS Windows, I presume).
> > >> I think the solution with a packaged breeze icons resource working
> > >> out-of-the box could be a good (lightweight) addition for non-Plasma
> > >> (non-Linux?) users of KF5, to popularize KF5. They grab the icons
> package
> >
> > Popularise, with Breeze "art"work? O:-)
> > Anyway,  I don't think "grabbing an icon package" will work on OS X, not
> if
> > you want to create standalone app bundles which by definition contain
> > everything they need.
> > >> and icons just work without thousands of files, caching, etc. 'One in
> a
> > >> million' would of these users would be interested in theming.
> >
> > I'd up that estimate if we're still talking about Breeze icons here O:-)
> >
> > >> PS2: I have been beaten by situations such as KToolBar setting 0-size
> > >> icons by default.
> >
> > Partly this is because almost no KF5 code uses the fallback argument of
> > QIcon::fromTheme() explicitly, which means that the function returns an
> > empty icon if the search fails. In particular, statements like
> >
> > app->setWindowIcon(QIcon::fromTheme(programName))
> >
> > should read
> >
> > app->setWindowIcon(QIcon::fromTheme(programName, app->windowIcon()))
> >
> > >> This is in a Windows env where no themes are (properly) installed. If
> the
> > >> rcc-based solution is in use I would imagine that ideally all the KF5
> > >> code
> > >> detects this somehow and would not look for xdg standard themes
> through
> > >> the
> > >> classic KIconLoader but silently adapt so the rcc resource works
> great.
> > >> Just a dream.
> >
> > If your rcc resource corresponds to the resource mentioned in the
> > QIcon::fromTheme() documentation (I think that talks about "qrc") and if
> I
> > interpret that documentation correctly then yes, code using that function
> > will find icons from the rcc/qrc "builtin" resource over those in xdg
> > themes (if the XDG icon repository is even in the icon theme search
> path).
> > >>> What I don't know however is whether artists consider that these
> icons
> > >>> should be themeable...
> >
> > I think icon artists will consider that you should touch their icons (for
> > theming or anything else). They will probably also consider that their
> > icons are the "best" but they really should also consider it a right for
> > anyone to use other icons ;)
> >
>
> The breeze.rcc file way is actually how Christoph solved it in Kate for
> Windows and OSX. We create an .rcc file from the breeze icons and at
> start-up
> we search for the file in QStandardPaths::DataLocation and if the file is
> found we load it with  registerResource() and add ":/icons" to the
> themeSearchPaths. (kate.git/icons.h)
>
> Christoph also added ":/icons" to the search path in kicontheme so that
> xmlgui
> toolbars and friends also get the icons from the .rcc file.
>
> We get all the same icons as on Linux neatly in one file :)
>

​Thanks, it works on Windows (no platform theme).
​​How to make it work on Linux too (KF5 5.19.0, Qt 5.5.1), i.e. I'd like to
have only used icons from the .rcc regardless of availability of
FrameworkIntegrationPlugin.so KDEPlatformTheme.so?
​
We're talking about this code:​

​https://quickgit.kde.org/?p=kate.git&a=blob&f=icons.h​​​

As soon as QResource::registerResource("breeze.rcc") is called, ":/icons"
is
​appended to the theme search path (checked using
QIcon::themeSearchPaths()).

There's a line QIcon::setThemeSearchPaths(QStringList() <<
QStringLiteral(":/icons")).

- When I have this line enabled and I have no FrameworkIntegrationPlugin.so
& KDEPlatformTheme.so installed, no icons are available at all

- When I have this line disabled, breeze XDG theme is used but kexi icons
(from the .rcc file are missing), regardless of availability of
FrameworkIntegrationPlugin.so & KDEPlatformTheme.so.


​For the record, Qt 5.5's code is:​

​QIcon QIcon::fromTheme(const QString &name, const QIcon &fallback)

Re: Review Request 127191: KCompletionBox should *not* be a tooltip

[Marco Martin]

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



any updates?

- Marco Martin


On Feb. 26, 2016, 2:18 p.m., Marco Martin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127191/
> ---
> 
> (Updated Feb. 26, 2016, 2:18 p.m.)
> 
> 
> Review request for KDE Frameworks and kwin.
> 
> 
> Repository: kcompletion
> 
> 
> Description
> ---
> 
> KCompletionbox it's actually more of  a combobox popup than a tooltip.
> this sets it the proper type (unfortunately with an hack due the type not 
> being available to Qt::WindowFlags)
> 
> without this, the new morphingpopups KWin effect will animate completion 
> boxes (such as in file open dialog, kate, dolphin adress bar etc)
> 
> 
> Diffs
> -
> 
>   src/kcompletionbox.cpp 99afc8e 
> 
> Diff: https://git.reviewboard.kde.org/r/127191/diff/
> 
> 
> Testing
> ---
> 
> Same behavior, due to the Window and bypasswindowmanager that are those two 
> that made the desired behavior when tooltip was picked
> 
> 
> Thanks,
> 
> Marco Martin
> 
>

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

Re: Review Request 127191: KCompletionBox should *not* be a tooltip

[Thomas Lübking]

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




src/kcompletionbox.cpp (line 65)


I doubt this is still required on top, but other than this it's oc. fine 
from my POV, tooltip has been the wrong type for a combo dropdown to begin with.


- Thomas Lübking


On Feb. 26, 2016, 2:18 p.m., Marco Martin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127191/
> ---
> 
> (Updated Feb. 26, 2016, 2:18 p.m.)
> 
> 
> Review request for KDE Frameworks and kwin.
> 
> 
> Repository: kcompletion
> 
> 
> Description
> ---
> 
> KCompletionbox it's actually more of  a combobox popup than a tooltip.
> this sets it the proper type (unfortunately with an hack due the type not 
> being available to Qt::WindowFlags)
> 
> without this, the new morphingpopups KWin effect will animate completion 
> boxes (such as in file open dialog, kate, dolphin adress bar etc)
> 
> 
> Diffs
> -
> 
>   src/kcompletionbox.cpp 99afc8e 
> 
> Diff: https://git.reviewboard.kde.org/r/127191/diff/
> 
> 
> Testing
> ---
> 
> Same behavior, due to the Window and bypasswindowmanager that are those two 
> that made the desired behavior when tooltip was picked
> 
> 
> Thanks,
> 
> Marco Martin
> 
>

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

Re: Review Request 127191: KCompletionBox should *not* be a tooltip

[Martin Gräßlin]

> On Feb. 26, 2016, 3:14 p.m., Thomas Lübking wrote:
> > src/kcompletionbox.cpp, line 66
> > 
> >
> > q->setAttribute(Qt::WA_X11NetWmWindowTypeCombo); // broken??
> 
> Marco Martin wrote:
> ok, i'm blind :p

cd src/qt5/qtbase/src/plugins/platforms/xcb
git grep X11NetWmWindowTypeCombo

-> no result

I doubt this still works...


- Martin


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


On Feb. 26, 2016, 3:18 p.m., Marco Martin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127191/
> ---
> 
> (Updated Feb. 26, 2016, 3:18 p.m.)
> 
> 
> Review request for KDE Frameworks and kwin.
> 
> 
> Repository: kcompletion
> 
> 
> Description
> ---
> 
> KCompletionbox it's actually more of  a combobox popup than a tooltip.
> this sets it the proper type (unfortunately with an hack due the type not 
> being available to Qt::WindowFlags)
> 
> without this, the new morphingpopups KWin effect will animate completion 
> boxes (such as in file open dialog, kate, dolphin adress bar etc)
> 
> 
> Diffs
> -
> 
>   src/kcompletionbox.cpp 99afc8e 
> 
> Diff: https://git.reviewboard.kde.org/r/127191/diff/
> 
> 
> Testing
> ---
> 
> Same behavior, due to the Window and bypasswindowmanager that are those two 
> that made the desired behavior when tooltip was picked
> 
> 
> Thanks,
> 
> Marco Martin
> 
>

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

Re: Review Request 127191: KCompletionBox should *not* be a tooltip

[Marco Martin]

> On Feb. 26, 2016, 2:14 p.m., Thomas Lübking wrote:
> > src/kcompletionbox.cpp, line 66
> > 
> >
> > q->setAttribute(Qt::WA_X11NetWmWindowTypeCombo); // broken??
> 
> Marco Martin wrote:
> ok, i'm blind :p
> 
> Martin Gräßlin wrote:
> cd src/qt5/qtbase/src/plugins/platforms/xcb
> git grep X11NetWmWindowTypeCombo
> 
> -> no result
> 
> I doubt this still works...

yep, indeed i get type normal now, getting back on the first approach


- Marco


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


On Feb. 26, 2016, 2:18 p.m., Marco Martin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127191/
> ---
> 
> (Updated Feb. 26, 2016, 2:18 p.m.)
> 
> 
> Review request for KDE Frameworks and kwin.
> 
> 
> Repository: kcompletion
> 
> 
> Description
> ---
> 
> KCompletionbox it's actually more of  a combobox popup than a tooltip.
> this sets it the proper type (unfortunately with an hack due the type not 
> being available to Qt::WindowFlags)
> 
> without this, the new morphingpopups KWin effect will animate completion 
> boxes (such as in file open dialog, kate, dolphin adress bar etc)
> 
> 
> Diffs
> -
> 
>   src/kcompletionbox.cpp 99afc8e 
> 
> Diff: https://git.reviewboard.kde.org/r/127191/diff/
> 
> 
> Testing
> ---
> 
> Same behavior, due to the Window and bypasswindowmanager that are those two 
> that made the desired behavior when tooltip was picked
> 
> 
> Thanks,
> 
> Marco Martin
> 
>

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

Re: Review Request 127191: KCompletionBox should *not* be a tooltip

[Marco Martin]

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

(Updated March 7, 2016, 11:35 a.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks and kwin.


Changes
---

Submitted with commit a4aced126beb14adc539edcb21e2423dd82ab6b0 by Marco Martin 
to branch master.


Repository: kcompletion


Description
---

KCompletionbox it's actually more of  a combobox popup than a tooltip.
this sets it the proper type (unfortunately with an hack due the type not being 
available to Qt::WindowFlags)

without this, the new morphingpopups KWin effect will animate completion boxes 
(such as in file open dialog, kate, dolphin adress bar etc)


Diffs
-

  src/kcompletionbox.cpp 99afc8e 

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


Testing
---

Same behavior, due to the Window and bypasswindowmanager that are those two 
that made the desired behavior when tooltip was picked


Thanks,

Marco Martin

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

Re: Review Request 127191: KCompletionBox should *not* be a tooltip

[Thomas Lübking]

> On Feb. 26, 2016, 2:14 p.m., Thomas Lübking wrote:
> > src/kcompletionbox.cpp, line 66
> > 
> >
> > q->setAttribute(Qt::WA_X11NetWmWindowTypeCombo); // broken??
> 
> Marco Martin wrote:
> ok, i'm blind :p
> 
> Martin Gräßlin wrote:
> cd src/qt5/qtbase/src/plugins/platforms/xcb
> git grep X11NetWmWindowTypeCombo
> 
> -> no result
> 
> I doubt this still works...
> 
> Marco Martin wrote:
> yep, indeed i get type normal now, getting back on the first approach

QComboBox still sets the attribute but it's in a "Q_DEAD_CODE_FROM_QT4_X11" 
preproc branch.
Qt5 is a piece of broken shit - seriously!


QComboBox would then fall to ::setWindowFlags(Qt::Popup) but actually that's 
not worth it at all. The target is QML + Phones, the rest of Qt is apparently 
now unmaintained junk.


- Thomas


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


On March 7, 2016, 11:35 a.m., Marco Martin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127191/
> ---
> 
> (Updated March 7, 2016, 11:35 a.m.)
> 
> 
> Review request for KDE Frameworks and kwin.
> 
> 
> Repository: kcompletion
> 
> 
> Description
> ---
> 
> KCompletionbox it's actually more of  a combobox popup than a tooltip.
> this sets it the proper type (unfortunately with an hack due the type not 
> being available to Qt::WindowFlags)
> 
> without this, the new morphingpopups KWin effect will animate completion 
> boxes (such as in file open dialog, kate, dolphin adress bar etc)
> 
> 
> Diffs
> -
> 
>   src/kcompletionbox.cpp 99afc8e 
> 
> Diff: https://git.reviewboard.kde.org/r/127191/diff/
> 
> 
> Testing
> ---
> 
> Same behavior, due to the Window and bypasswindowmanager that are those two 
> that made the desired behavior when tooltip was picked
> 
> 
> Thanks,
> 
> Marco Martin
> 
>

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

Re: RCC for icons - update: Re: Icons installed by apps

[Jaroslaw Staniek]

On 7 March 2016 at 10:46, Jaroslaw Staniek  wrote:

>
>
> On 18 February 2016 at 18:25, Kåre Särs  wrote:
>
>> On Thursday, February 18, 2016 02:51:58 PM René J.V. Bertin wrote:
>> > On Thursday February 18 2016 13:55:18 Jaroslaw Staniek wrote:
>> > >>> > since QIcon::fromTheme() apaprently isn't able to find app icons.
>> >
>> > Care to explain? QIcon::fromTheme() doesn't find anything "out of the
>> box"
>> > on OS X (and I presume on MS Windows), but that is only because no theme
>> > search path is set on those platforms. When you add the standard XDG
>> icon
>> > repository to the icon search path on OS X, even "pure" Qt5 application
>> > will start showing icons all over if you have an icon theme installed --
>> > including in widgets that should not have icons according to the HIG.
>> Also
>> > on OS X, fromTheme() will only return the application icon (as in the
>> icon
>> > shown in the Finder) if the current theme defines that same icon for the
>> > calling application, and the theme search path is set of course. In all
>> > other cases it will not, because the application icon is not defined
>> > through a theme on OS X (nor is it on MS Windows, I presume).
>> > >> I think the solution with a packaged breeze icons resource working
>> > >> out-of-the box could be a good (lightweight) addition for non-Plasma
>> > >> (non-Linux?) users of KF5, to popularize KF5. They grab the icons
>> package
>> >
>> > Popularise, with Breeze "art"work? O:-)
>> > Anyway,  I don't think "grabbing an icon package" will work on OS X,
>> not if
>> > you want to create standalone app bundles which by definition contain
>> > everything they need.
>> > >> and icons just work without thousands of files, caching, etc. 'One
>> in a
>> > >> million' would of these users would be interested in theming.
>> >
>> > I'd up that estimate if we're still talking about Breeze icons here O:-)
>> >
>> > >> PS2: I have been beaten by situations such as KToolBar setting 0-size
>> > >> icons by default.
>> >
>> > Partly this is because almost no KF5 code uses the fallback argument of
>> > QIcon::fromTheme() explicitly, which means that the function returns an
>> > empty icon if the search fails. In particular, statements like
>> >
>> > app->setWindowIcon(QIcon::fromTheme(programName))
>> >
>> > should read
>> >
>> > app->setWindowIcon(QIcon::fromTheme(programName, app->windowIcon()))
>> >
>> > >> This is in a Windows env where no themes are (properly) installed.
>> If the
>> > >> rcc-based solution is in use I would imagine that ideally all the KF5
>> > >> code
>> > >> detects this somehow and would not look for xdg standard themes
>> through
>> > >> the
>> > >> classic KIconLoader but silently adapt so the rcc resource works
>> great.
>> > >> Just a dream.
>> >
>> > If your rcc resource corresponds to the resource mentioned in the
>> > QIcon::fromTheme() documentation (I think that talks about "qrc") and
>> if I
>> > interpret that documentation correctly then yes, code using that
>> function
>> > will find icons from the rcc/qrc "builtin" resource over those in xdg
>> > themes (if the XDG icon repository is even in the icon theme search
>> path).
>> > >>> What I don't know however is whether artists consider that these
>> icons
>> > >>> should be themeable...
>> >
>> > I think icon artists will consider that you should touch their icons
>> (for
>> > theming or anything else). They will probably also consider that their
>> > icons are the "best" but they really should also consider it a right for
>> > anyone to use other icons ;)
>> >
>>
>> The breeze.rcc file way is actually how Christoph solved it in Kate for
>> Windows and OSX. We create an .rcc file from the breeze icons and at
>> start-up
>> we search for the file in QStandardPaths::DataLocation and if the file is
>> found we load it with  registerResource() and add ":/icons" to the
>> themeSearchPaths. (kate.git/icons.h)
>>
>> Christoph also added ":/icons" to the search path in kicontheme so that
>> xmlgui
>> toolbars and friends also get the icons from the .rcc file.
>>
>> We get all the same icons as on Linux neatly in one file :)
>>
>
> ​Thanks, it works on Windows (no platform theme).
> ​​How to make it work on Linux too (KF5 5.19.0, Qt 5.5.1), i.e. I'd like
> to have only used icons from the .rcc regardless of availability of
> FrameworkIntegrationPlugin.so KDEPlatformTheme.so?
> ​
> We're talking about this code:​
>
> ​https://quickgit.kde.org/?p=kate.git&a=blob&f=icons.h​​​
>
> As soon as QResource::registerResource("breeze.rcc") is called, ":/icons"
> is
> ​appended to the theme search path (checked using
> QIcon::themeSearchPaths()).
>
> There's a line QIcon::setThemeSearchPaths(QStringList() <<
> QStringLiteral(":/icons")).
>
> - When I have this line enabled and I have no
> FrameworkIntegrationPlugin.so & KDEPlatformTheme.so installed, no icons are
> available at all
>
> - When I have this line disabled, breeze XDG theme is used but kexi icons
> (from the .rcc file are m

Re: Review Request 127275: Ki18n: Fallback to QLocale::system uiLanguages in language initalisation

[Andre Heinecke]

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

(Updated March 7, 2016, 12:32 p.m.)


Review request for KDE Frameworks and Localization and Translation (l10n).


Changes
---

Added ifndef Q_OS_UNIX around fallback.


Repository: ki18n


Description
---

The intention for this patch is to fix the inital Language selection for 
Windows where the environment variables used in Ki18n are not set.
This is not a fix for a regression in Ki18n, afaik this never worked on 
Windows, we had some hacks in Gpg4win to write the language into kdeglobals 
during installation in kde4 times.

I don't think this needs to be ifdefed because it only appends so previous 
language selection is not affected.


Diffs (updated)
-

  src/klocalizedstring.cpp b24fe9b 

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


Testing
---

Tested on a german Windows system and got a "de" localized application.


Thanks,

Andre Heinecke

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

Re: RCC for icons - update: Re: Icons installed by apps

[René J . V . Bertin]

On Monday March 07 2016 12:41:52 Jaroslaw Staniek wrote:
>I am trying to use app-defined icons through QIcon::fromTheme() in Kexi.

That sounds inherently wrong unless the application adds icons to specific 
themes. Icons that are specific to an application are (almost) by definition 
not part of a theme, so expecting QIcon::fromTheme() to return them seems a bit 
of a misunderstanding.

>Without that I'd have to duplicate logic of icon themes just to make 
>QIcon::fromTheme() work cross-platform.

Why? Why not do as Kate and use QIcon(":/icon-from-rcc") instead of 
QIcon::fromTheme() for app-specific icons that should be independent of the 
user's icon theme choice, and ::fromTheme() for those icons that are supposed 
to reflect his/her choice of theme?

I don't think there's any need whatsoever to duplicate (reimplement) the logic 
of icon themes. AFAIK, QIcon::fromTheme() will work anywhere once you define an 
icon theme search path and the expected icon theme is installed somewhere in 
that path.

BTW, am I right that using a builtin binary rcc icon set could make you lose in 
terms of memory (RAM) footprint overhead what you gain in terms of disk space 
overhead?

R

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

Re: Review Request 127251: [KUnitConversion] Fix downloading currency exchange rates

[Kai Uwe Broulik]

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

(Updated März 7, 2016, 2:34 nachm.)


Review request for KDE Frameworks and Vishesh Handa.


Changes
---

Exclude user inputs


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


Repository: kunitconversion


Description
---

QNetworkReply does not implement waitForReadyRead

Also, the code never actually created the cache dir it was trying to create a 
file in.


Diffs (updated)
-

  src/currency.cpp 3b99644 
  src/unit.h 9e17624 

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


Testing
---

Works now. It's downloaded once and then taken from cache file in 
~/.local/share/libkunitconversion/currency.xml

Given it's a Tier 2 framework doesn't make sense to add KIO now, also failed to 
reproduce the crashes mentioned in the code.

Tests pass (only if I run them on English locale btw)

Obviously not happy with this being sync but alas that's how the API works.

Not sure if this is a KRunner bug or KUnitConverison but if I enter "5 USD" it 
converts fine, if I enter "5 usd" it returns zero for all the currencies it 
converted to.


Thanks,

Kai Uwe Broulik

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

Re: Review Request 127251: [KUnitConversion] Fix downloading currency exchange rates

[Kai Uwe Broulik]

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

(Updated März 7, 2016, 2:35 nachm.)


Review request for KDE Frameworks and Vishesh Handa.


Changes
---

Remove cruft from other review


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


Repository: kunitconversion


Description
---

QNetworkReply does not implement waitForReadyRead

Also, the code never actually created the cache dir it was trying to create a 
file in.


Diffs (updated)
-

  src/currency.cpp 3b99644 

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


Testing
---

Works now. It's downloaded once and then taken from cache file in 
~/.local/share/libkunitconversion/currency.xml

Given it's a Tier 2 framework doesn't make sense to add KIO now, also failed to 
reproduce the crashes mentioned in the code.

Tests pass (only if I run them on English locale btw)

Obviously not happy with this being sync but alas that's how the API works.

Not sure if this is a KRunner bug or KUnitConverison but if I enter "5 USD" it 
converts fine, if I enter "5 usd" it returns zero for all the currencies it 
converted to.


Thanks,

Kai Uwe Broulik

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

Re: Review Request 127166: Fix xcb port of klauncher and clean up the code.

[Martin Gräßlin]

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


Ship it!




Ship It!

- Martin Gräßlin


On March 4, 2016, 11:51 p.m., Xuetian Weng wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127166/
> ---
> 
> (Updated March 4, 2016, 11:51 p.m.)
> 
> 
> Review request for KDE Frameworks, David Faure and Martin Gräßlin.
> 
> 
> Repository: kinit
> 
> 
> Description
> ---
> 
> The ported klauncher at least has two bugs:
> 1. IMHO, it should compare the cached display string with == instead of != 
> (though != has been there since the old kdelibs, but it just doesn't look 
> correct to me, what's the point of assign dpy = mCached_dpy when dpy_str != 
> XDisplayString(dpy)? And what's the point use == in one place and != in other 
> two places? ).
> 2. it might free the cached connection without setting mCached.conn back to 
> nullptr, which could lead to double free or freeze when system logout.
> 
> This patch refactor the code a little bit with a helper function to update 
> the cached connection instead of duplicate the same logic everywhere. 
> getXCBConnection() will make sure the returned connection is error-free, 
> reuse the cached connection if possible, and update the cached connection if 
> needed.
> 
> 
> Diffs
> -
> 
>   src/klauncher/klauncher.h 31bfaaa 
>   src/klauncher/klauncher.cpp 7ea9da9 
> 
> Diff: https://git.reviewboard.kde.org/r/127166/diff/
> 
> 
> Testing
> ---
> 
> Compiles, startup notify works.
> 
> 
> Thanks,
> 
> Xuetian Weng
> 
>

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

Re: RCC for icons - update: Re: Icons installed by apps

[Jaroslaw Staniek]

On 7 March 2016 at 14:45, René J.V.  wrote:

> On Monday March 07 2016 12:41:52 Jaroslaw Staniek wrote:
> >I am trying to use app-defined icons through QIcon::fromTheme() in Kexi.
>
> That sounds inherently wrong unless the application adds icons to specific
> themes. Icons that are specific to an application are (almost) by
> definition not part of a theme, so expecting QIcon::fromTheme() to return
> them seems a bit of a misunderstanding.
>

​I am using the icons theme infra for this. Alternative would be to
duplicate the code to achieve exactly the same. QIcon(filename) does not
even support multiple sizes; you need to code this. Note how a Kate plugin
I mentioned above uses hardcoded
":/katesql/pics/16-actions-sql-field-pk.png" file.
Please also note I am not mixing breeze theme and app's breeze icons. They
are separated.


> >Without that I'd have to duplicate logic of icon themes just to make
> QIcon::fromTheme() work cross-platform.
>
> Why? Why not do as Kate and use QIcon(":/icon-from-rcc") instead of
> QIcon::fromTheme() for app-specific icons that should be independent of the
> user's icon theme choice, and ::fromTheme() for those icons that are
> supposed to reflect his/her choice of theme?
>

Because ​I am not reflecting the choice in Kexi's icons. The only that are
produced (takes weeks) and referenced in docs are the breeze ones. Anyone
is free to start project aimed at supporting another theme. This is a
switch in philosophy to boost the quality, and I accept you may disagree.
But how icons are packaged distributed should reflect the development
process and priorities of the app project.


>
> I don't think there's any need whatsoever to duplicate (reimplement) the
> logic of icon themes. AFAIK, QIcon::fromTheme() will work anywhere once you
> define an icon theme search path and the expected icon theme is installed
> somewhere in that path.
>
> BTW, am I right that using a builtin binary rcc icon set could make you
> lose in terms of memory (RAM) footprint overhead what you gain in terms of
> disk space overhead?
>

With all due respect. ​Dunno. I am writing this email in a 2GiB large email
client (GMail in Firefox).​
With thousands of cached icons and copies of jQuery. We're still super
light.

As David Faure said not once here, technically we just don't have KDE apps
anymore. We have Qt apps. We can't assume themes are installed, properly
installed, or caching is in place. Optimizations is the 1% remaining thing.
I know no user who abandons software because of such things. Mac had
universal binaries for years, with interesting growth in size. FatELF,
application containers, all these are approaches that spend a few bytes in
exchange for convenience, security, etc.

​My kexi_breeze.rcc is 184​
​k an full (unoptimized) breeze.rcc is 14M compressed 35MiB fully
uncompressed, while my wallpapers on just 2 screens are 18M compressed,
~120MiB in graphics RAM. And many of the icon file take >=4096 bytes
despite being 2048 bytes large.
I guess you also know that random-accessed files are mmapped and read on
demand. Though  I bet a 14M file will be read-ahead on any todays system.

2016 calling :) The resource files are easily handled in my years-old
smartphone, so...

There's nearly zero stat() calls for icons discovery instead of thousands
per (even tiny) app.
And packagers can easily package breeze.rcc (I'd recommend this in a
README.PACKAGERS file).
​
-- 
regards, Jaroslaw Staniek

KDE:
: A world-wide network of software engineers, artists, writers, translators
: and facilitators committed to Free Software development - http://kde.org
Calligra Suite:
: A graphic art and office suite - http://calligra.org
Kexi:
: A visual database apps builder - http://calligra.org/kexi
Qt Certified Specialist:
: http://www.linkedin.com/in/jstaniek
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127275: Ki18n: Fallback to QLocale::system uiLanguages in language initalisation

[Chusslove Illich]

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


Ship it!




Ship It!

- Chusslove Illich


On Март 7, 2016, 1:32 по п., Andre Heinecke wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127275/
> ---
> 
> (Updated Март 7, 2016, 1:32 по п.)
> 
> 
> Review request for KDE Frameworks and Localization and Translation (l10n).
> 
> 
> Repository: ki18n
> 
> 
> Description
> ---
> 
> The intention for this patch is to fix the inital Language selection for 
> Windows where the environment variables used in Ki18n are not set.
> This is not a fix for a regression in Ki18n, afaik this never worked on 
> Windows, we had some hacks in Gpg4win to write the language into kdeglobals 
> during installation in kde4 times.
> 
> I don't think this needs to be ifdefed because it only appends so previous 
> language selection is not affected.
> 
> 
> Diffs
> -
> 
>   src/klocalizedstring.cpp b24fe9b 
> 
> Diff: https://git.reviewboard.kde.org/r/127275/diff/
> 
> 
> Testing
> ---
> 
> Tested on a german Windows system and got a "de" localized application.
> 
> 
> Thanks,
> 
> Andre Heinecke
> 
>

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

Re: RCC for icons - update: Re: Icons installed by apps

[Jaroslaw Staniek]

Real data for a single Kexi start + opening a single document on Linux, SSD:

strace kexi 2>&1 | grep ^stat | wc -l

1. Traditional icon files: 78k calls
2. .rcc icon files: 11k calls, starts ~14% faster

And for #2 there are still a few thousands of lookups
(*/icons/hicolor/16x16/devices etc.) for icons probably at KF5 and KIO
level and alike - they can be reduced only if traditional icon files are
uninstalled or installed away from the default search path.

Each stat() on Windows would take more time.



On 7 March 2016 at 15:53, Jaroslaw Staniek  wrote:

>
>
> On 7 March 2016 at 14:45, René J.V.  wrote:
>
>> On Monday March 07 2016 12:41:52 Jaroslaw Staniek wrote:
>> >I am trying to use app-defined icons through QIcon::fromTheme() in Kexi.
>>
>> That sounds inherently wrong unless the application adds icons to
>> specific themes. Icons that are specific to an application are (almost) by
>> definition not part of a theme, so expecting QIcon::fromTheme() to return
>> them seems a bit of a misunderstanding.
>>
>
> ​I am using the icons theme infra for this. Alternative would be to
> duplicate the code to achieve exactly the same. QIcon(filename) does not
> even support multiple sizes; you need to code this. Note how a Kate plugin
> I mentioned above uses hardcoded
> ":/katesql/pics/16-actions-sql-field-pk.png" file.
> Please also note I am not mixing breeze theme and app's breeze icons. They
> are separated.
>
>
>> >Without that I'd have to duplicate logic of icon themes just to make
>> QIcon::fromTheme() work cross-platform.
>>
>> Why? Why not do as Kate and use QIcon(":/icon-from-rcc") instead of
>> QIcon::fromTheme() for app-specific icons that should be independent of the
>> user's icon theme choice, and ::fromTheme() for those icons that are
>> supposed to reflect his/her choice of theme?
>>
>
> Because ​I am not reflecting the choice in Kexi's icons. The only that are
> produced (takes weeks) and referenced in docs are the breeze ones. Anyone
> is free to start project aimed at supporting another theme. This is a
> switch in philosophy to boost the quality, and I accept you may disagree.
> But how icons are packaged distributed should reflect the development
> process and priorities of the app project.
>
>
>>
>> I don't think there's any need whatsoever to duplicate (reimplement) the
>> logic of icon themes. AFAIK, QIcon::fromTheme() will work anywhere once you
>> define an icon theme search path and the expected icon theme is installed
>> somewhere in that path.
>>
>> BTW, am I right that using a builtin binary rcc icon set could make you
>> lose in terms of memory (RAM) footprint overhead what you gain in terms of
>> disk space overhead?
>>
>
> With all due respect. ​Dunno. I am writing this email in a 2GiB large
> email client (GMail in Firefox).​
> With thousands of cached icons and copies of jQuery. We're still super
> light.
>
> As David Faure said not once here, technically we just don't have KDE apps
> anymore. We have Qt apps. We can't assume themes are installed, properly
> installed, or caching is in place. Optimizations is the 1% remaining thing.
> I know no user who abandons software because of such things. Mac had
> universal binaries for years, with interesting growth in size. FatELF,
> application containers, all these are approaches that spend a few bytes in
> exchange for convenience, security, etc.
>
> ​My kexi_breeze.rcc is 184​
> ​k an full (unoptimized) breeze.rcc is 14M compressed 35MiB fully
> uncompressed, while my wallpapers on just 2 screens are 18M compressed,
> ~120MiB in graphics RAM. And many of the icon file take >=4096 bytes
> despite being 2048 bytes large.
> I guess you also know that random-accessed files are mmapped and read on
> demand. Though  I bet a 14M file will be read-ahead on any todays system.
>
> 2016 calling :) The resource files are easily handled in my years-old
> smartphone, so...
>
> There's nearly zero stat() calls for icons discovery instead of thousands
> per (even tiny) app.
> And packagers can easily package breeze.rcc (I'd recommend this in a
> README.PACKAGERS file).
> ​
> --
> regards, Jaroslaw Staniek
>
> KDE:
> : A world-wide network of software engineers, artists, writers, translators
> : and facilitators committed to Free Software development - http://kde.org
> Calligra Suite:
> : A graphic art and office suite - http://calligra.org
> Kexi:
> : A visual database apps builder - http://calligra.org/kexi
> Qt Certified Specialist:
> : http://www.linkedin.com/in/jstaniek
>



-- 
regards, Jaroslaw Staniek

KDE:
: A world-wide network of software engineers, artists, writers, translators
: and facilitators committed to Free Software development - http://kde.org
Calligra Suite:
: A graphic art and office suite - http://calligra.org
Kexi:
: A visual database apps builder - http://calligra.org/kexi
Qt Certified Specialist:
: http://www.linkedin.com/in/jstaniek
___
Kde-frameworks-devel mailing list
Kde-f

Re: RCC for icons - update: Re: Icons installed by apps

[Jaroslaw Staniek]

On 7 March 2016 at 13:05, Kåre Särs  wrote:

> Hi,
>
> On Monday, March 07, 2016 12:41:52 PM Jaroslaw Staniek wrote:
> > >
> > > ​Thanks, it works on Windows (no platform theme).
> > > ​​How to make it work on Linux too (KF5 5.19.0, Qt 5.5.1), i.e. I'd
> like
> > > to have only used icons from the .rcc regardless of availability of
> > > FrameworkIntegrationPlugin.so KDEPlatformTheme.so?
> > > ​
> > > We're talking about this code:​
> > >
> > > ​https://quickgit.kde.org/?p=kate.git&a=blob&f=icons.h​​​
> > >
> > > As soon as QResource::registerResource("breeze.rcc") is called,
> ":/icons"
> > > is
> > > ​appended to the theme search path (checked using
> > > QIcon::themeSearchPaths()).
> > >
> > > There's a line QIcon::setThemeSearchPaths(QStringList() <<
> > > QStringLiteral(":/icons")).
> > >
> > > - When I have this line enabled and I have no
> > > FrameworkIntegrationPlugin.so & KDEPlatformTheme.so installed, no icons
> > > are
> > > available at all
>
> Notice that the setThemeSearchPaths() call _replaces_ any previous search
> paths. Is this your problem?
>
>
>
​No.. but I got it sorted out after reading QIconTheme::QIconTheme and
alike and re-reading the "The name should correspond to a directory name in
the themeSearchPath() containing an index.theme file describing it's
contents" sentence of  QIcon::setThemeName docs :)

I was tricked by the fact that on Windows qrc files with these paths work:

./icons/actions/16/code-block.svg
./icons/actions/16/code-class.svg
etc.

And not on Linux. No idea why. Here I needed:

./icons/breeze/actions/16/code-block.svg
./icons/breeze/actions/16/code-class.svg

Of course also, otherwise the theme is marked as invalid
./icons/breeze/index.theme

(needed once; if you have the index.theme accessible from breeze.rcc, you
can add an extra application-defined .rcc without index.theme and new icons
will just be "added" to the theme. Looks like useful thing for plugins for
example, shipped completely separate from distros, and maybe not even
packaged. Just make sure you don't use any new subdirs that are are not
defined by the index file (I guess).

Now useful thing would be to have option to generate breeze.rcc directly in
breeze-icons.git, so apps can find breeze.rcc at configure time, e.g. using
FindBreezeIcons.cmake.

Thanks, sorry for the noise. All the relevant recipes would end up in a
howto and/or cmake scripts.
​

-- 
regards, Jaroslaw Staniek

KDE:
: A world-wide network of software engineers, artists, writers, translators
: and facilitators committed to Free Software development - http://kde.org
Calligra Suite:
: A graphic art and office suite - http://calligra.org
Kexi:
: A visual database apps builder - http://calligra.org/kexi
Qt Certified Specialist:
: http://www.linkedin.com/in/jstaniek
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: RCC for icons - update: Re: Icons installed by apps

[Jaroslaw Staniek]

On 7 March 2016 at 14:45, René J.V.  wrote:

> On Monday March 07 2016 12:41:52 Jaroslaw Staniek wrote:
>
> BTW, am I right that using a builtin binary rcc icon set could make you
> lose in terms of memory (RAM) footprint overhead what you gain in terms of
> disk space overhead?
>

Dsk optimization is not the goal, that would be Enlightenment-like
optimization :)
The thing is, as an average Qt dev, if I had to develop and distribute a 5
cpp files-long lib or app, self-contained (not packaged), I wouldn't like
to add 5970 (and counting) icon files to it to just display 3 icons or so.
I would probably copy the  needed icons into a subdir, maybe even rename
them, move around, etc.
Disclaimer: we're not 'average' Qt devs with all consequences but my dream
is that the two extremes can be closer.

I've been doing the copying myself for non-FOSS or non-mainstream FOSS
code. So any improvement like having a single breeze.rcc file is more than
good solution IMHO.
In other words, we're not distributing movies in a form of N jpeg files,
one per frame, we're using dedicated container. So here it is, a dedicated
container for icons.

-- 
regards, Jaroslaw Staniek

KDE:
: A world-wide network of software engineers, artists, writers, translators
: and facilitators committed to Free Software development - http://kde.org
Calligra Suite:
: A graphic art and office suite - http://calligra.org
Kexi:
: A visual database apps builder - http://calligra.org/kexi
Qt Certified Specialist:
: http://www.linkedin.com/in/jstaniek
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: RCC for icons - update: Re: Icons installed by apps

[Jaroslaw Staniek]

On 7 March 2016 at 17:29, Jaroslaw Staniek  wrote:

>
> On 7 March 2016 at 13:05, Kåre Särs  wrote:
>
>> Hi,
>>
>> On Monday, March 07, 2016 12:41:52 PM Jaroslaw Staniek wrote:
>> > >
>> > > ​Thanks, it works on Windows (no platform theme).
>> > > ​​How to make it work on Linux too (KF5 5.19.0, Qt 5.5.1), i.e. I'd
>> like
>> > > to have only used icons from the .rcc regardless of availability of
>> > > FrameworkIntegrationPlugin.so KDEPlatformTheme.so?
>> > > ​
>> > > We're talking about this code:​
>> > >
>> > > ​https://quickgit.kde.org/?p=kate.git&a=blob&f=icons.h​​​
>> > >
>> > > As soon as QResource::registerResource("breeze.rcc") is called,
>> ":/icons"
>> > > is
>> > > ​appended to the theme search path (checked using
>> > > QIcon::themeSearchPaths()).
>> > >
>> > > There's a line QIcon::setThemeSearchPaths(QStringList() <<
>> > > QStringLiteral(":/icons")).
>> > >
>> > > - When I have this line enabled and I have no
>> > > FrameworkIntegrationPlugin.so & KDEPlatformTheme.so installed, no
>> icons
>> > > are
>> > > available at all
>>
>> Notice that the setThemeSearchPaths() call _replaces_ any previous search
>> paths. Is this your problem?
>>
>>
>>
> ​No.. but I got it sorted out after reading QIconTheme::QIconTheme and
> alike and re-reading the "The name should correspond to a directory name in
> the themeSearchPath() containing an index.theme file describing it's
> contents" sentence of  QIcon::setThemeName docs :)
>


​PS: If someone wants to experiment (I hear it's the case for KDevelop),
here is the
Breeze icons resource: http://kexi-project.org/tmp/breeze.rcc.bz2

For reference, Kexi icons: http://kexi-project.org/tmp/kexi_breeze.rcc.bz2
And recent commit that supports it all on Linux and Windows:
https://quickgit.kde.org/?p=scratch%2Fstaniek%2Fkexi3.git

-- 
regards, Jaroslaw Staniek

KDE:
: A world-wide network of software engineers, artists, writers, translators
: and facilitators committed to Free Software development - http://kde.org
Calligra Suite:
: A graphic art and office suite - http://calligra.org
Kexi:
: A visual database apps builder - http://calligra.org/kexi
Qt Certified Specialist:
: http://www.linkedin.com/in/jstaniek
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127237: Fix crash in kmore tools on Windows

[Kåre Särs]

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

(Updated March 7, 2016, 8:38 p.m.)


Review request for KDE Frameworks and Gregor Mi.


Changes
---

It is not related to DBus (Just Windows ;)

Added comments and a nullptr check before adding a the pointer to the 
kmtServiceList


Summary (updated)
-

Fix crash in kmore tools on Windows


Repository: knewstuff


Description (updated)
---

On Windows we sometimes get null-pointers in stead of pointers to 
KMoreToolsService. This patch adds checks for null-pointers in those places 
that made Kate crash in the project plugin and adds a nullptr check before 
adding them to the list.


Diffs (updated)
-

  src/kmoretools/kmoretoolsmenufactory.cpp 30f4d02 
  src/kmoretools/kmoretoolspresets.cpp 2405321 

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


Testing (updated)
---

Compiled and run on windows. No crashes in Kate when right-clicking files in 
the project plugin.


Thanks,

Kåre Särs

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

Re: Review Request 127237: Fix crash in kmore tools on Windows

[Gregor Mi]

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




src/kmoretools/kmoretoolsmenufactory.cpp (lines 128 - 129)


Thanks for the comment. However, as long as we don't know the root cause 
for the null pointers, I would feel better if the comment states clearly that 
we don't know what is happening and that it is happening on Windows only.



src/kmoretools/kmoretoolsmenufactory.cpp (lines 237 - 238)


see my comment above



src/kmoretools/kmoretoolspresets.cpp (line 97)


Again, this should not happen here, so please add a comment that this is a 
fix for Windows.



src/kmoretools/kmoretoolspresets.cpp (lines 165 - 167)


As above: as long as we don't know the reason for the nullpointers I would 
be happier if there was a comment.

That said I don't know what the KDE policy is when dealing with this kind 
of problem. Just adding null checks seems to make the code more complicated to 
analyse later. You are a long-term committer, so your words have a high weight. 
On the other hand I would be interested in a second opinion.


- Gregor Mi


On March 7, 2016, 8:38 p.m., Kåre Särs wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127237/
> ---
> 
> (Updated March 7, 2016, 8:38 p.m.)
> 
> 
> Review request for KDE Frameworks and Gregor Mi.
> 
> 
> Repository: knewstuff
> 
> 
> Description
> ---
> 
> On Windows we sometimes get null-pointers in stead of pointers to 
> KMoreToolsService. This patch adds checks for null-pointers in those places 
> that made Kate crash in the project plugin and adds a nullptr check before 
> adding them to the list.
> 
> 
> Diffs
> -
> 
>   src/kmoretools/kmoretoolsmenufactory.cpp 30f4d02 
>   src/kmoretools/kmoretoolspresets.cpp 2405321 
> 
> Diff: https://git.reviewboard.kde.org/r/127237/diff/
> 
> 
> Testing
> ---
> 
> Compiled and run on windows. No crashes in Kate when right-clicking files in 
> the project plugin.
> 
> 
> Thanks,
> 
> Kåre Särs
> 
>

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

Re: Review Request 127166: Fix xcb port of klauncher and clean up the code.

[Xuetian Weng]

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

(Updated March 8, 2016, 12:17 a.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks, David Faure and Martin Gräßlin.


Changes
---

Submitted with commit d154b4c0d85c1616c512cabdf5bf9919a9a9191d by Weng Xuetian 
to branch master.


Repository: kinit


Description
---

The ported klauncher at least has two bugs:
1. IMHO, it should compare the cached display string with == instead of != 
(though != has been there since the old kdelibs, but it just doesn't look 
correct to me, what's the point of assign dpy = mCached_dpy when dpy_str != 
XDisplayString(dpy)? And what's the point use == in one place and != in other 
two places? ).
2. it might free the cached connection without setting mCached.conn back to 
nullptr, which could lead to double free or freeze when system logout.

This patch refactor the code a little bit with a helper function to update the 
cached connection instead of duplicate the same logic everywhere. 
getXCBConnection() will make sure the returned connection is error-free, reuse 
the cached connection if possible, and update the cached connection if needed.


Diffs
-

  src/klauncher/klauncher.h 31bfaaa 
  src/klauncher/klauncher.cpp 7ea9da9 

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


Testing
---

Compiles, startup notify works.


Thanks,

Xuetian Weng

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

Re: Review Request 127237: Fix crash in kmore tools on Windows

[Kåre Särs]

> On March 7, 2016, 9:29 p.m., Gregor Mi wrote:
> > src/kmoretools/kmoretoolsmenufactory.cpp, lines 128-129
> > 
> >
> > Thanks for the comment. However, as long as we don't know the root 
> > cause for the null pointers, I would feel better if the comment states 
> > clearly that we don't know what is happening and that it is happening on 
> > Windows only.

This one is not needed at the moment (I did not get null pointers here), but I 
left it there just in case. I feel it is better to loose functionality than 
getting a crash ;)


> On March 7, 2016, 9:29 p.m., Gregor Mi wrote:
> > src/kmoretools/kmoretoolsmenufactory.cpp, lines 237-238
> > 
> >
> > see my comment above

same as above ;)


> On March 7, 2016, 9:29 p.m., Gregor Mi wrote:
> > src/kmoretools/kmoretoolspresets.cpp, line 97
> > 
> >
> > Again, this should not happen here, so please add a comment that this 
> > is a fix for Windows.

This is the first place where we get the null pointers. 
KMoreTools::registerServiceByDesktopEntryName() has at least two code paths 
that return nullptr.
kmoretools.cpp
line 129: qCritical() << ... the kmt-desktopfile " << desktopEntryName << " is 
provided but no Exec line is specified ...
line 130: return nullptr;
and
line 146: qCritical() << ... a kmt-desktopfile must be provided. Please fix. 
Return nullptr. ...
line 147: return nullptr;

These might be errors, but I would rather see that it does not crash even if a 
runtime file is missing. I suspect that it is the later that generates the 
nullptr.
This is why I check the returned pointer before using it.

QStandardPaths::GenericDataLocation does not return ../share/ on Windows and 
thus does not find the file


> On March 7, 2016, 9:29 p.m., Gregor Mi wrote:
> > src/kmoretools/kmoretoolspresets.cpp, lines 165-167
> > 
> >
> > As above: as long as we don't know the reason for the nullpointers I 
> > would be happier if there was a comment.
> > 
> > That said I don't know what the KDE policy is when dealing with this 
> > kind of problem. Just adding null checks seems to make the code more 
> > complicated to analyse later. You are a long-term committer, so your words 
> > have a high weight. On the other hand I would be interested in a second 
> > opinion.

KMoreToolsPresets::registerServiceByDesktopEntryName() has one code path that 
returns a direct nullptr and another that calls 
KMoreTools::registerServiceByDesktopEntryName() that also can return a nullptr. 
So I cannot be sure that the pointer isn't null. So check it and don't add 
nullptr ;)

The problem is, with 99% certainty, the missing .desktop file, but IMO we 
should not crash even if a runtime file is missing ;)


- Kåre


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


On March 7, 2016, 8:38 p.m., Kåre Särs wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127237/
> ---
> 
> (Updated March 7, 2016, 8:38 p.m.)
> 
> 
> Review request for KDE Frameworks and Gregor Mi.
> 
> 
> Repository: knewstuff
> 
> 
> Description
> ---
> 
> On Windows we sometimes get null-pointers in stead of pointers to 
> KMoreToolsService. This patch adds checks for null-pointers in those places 
> that made Kate crash in the project plugin and adds a nullptr check before 
> adding them to the list.
> 
> 
> Diffs
> -
> 
>   src/kmoretools/kmoretoolsmenufactory.cpp 30f4d02 
>   src/kmoretools/kmoretoolspresets.cpp 2405321 
> 
> Diff: https://git.reviewboard.kde.org/r/127237/diff/
> 
> 
> Testing
> ---
> 
> Compiled and run on windows. No crashes in Kate when right-clicking files in 
> the project plugin.
> 
> 
> Thanks,
> 
> Kåre Särs
> 
>

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