Re: about ki18n/locales: installing only a subset?

[Chusslove Illich]

> [: René J.V. Bertin :]
> [...] AFAIK it's always been a habit among distributions to package
> languages separately.

Which distributions? Not the ones I use, Debian and Suse. I know only of
Ubuntu doing it.

>> [: Chusslove Illich :]
>> Furthermore, if one thinks of stripping the translation files from
>> release packages, or fetching them manually from the repo, that will not
>> work properly.
>
> [: René J.V. Bertin :]
> It worked just fine with KDE4, esp. with the fallback feature to a
> language that isn't automatically the developer's language (usually
> en_US).

I meant whoever thinks doing that on his own. In KDE4 it worked because the
language packages were officially provided.

And it only worked in the sense that all files were accounted for. In
practice I had only frustration with language packages, having to pay
attention to install them as well when provided separately. The disk space
overhead is preferable to having to think about it, and highly preferable to
bothering any admins about it.

-- 
Chusslove Illich (Часлав Илић)


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: Draft split for qpt plugin from frameworkintegration

[Martin Graesslin]

On Thursday, December 17, 2015 5:48:47 PM CET Martin Graesslin wrote:
> On Thursday, December 17, 2015 4:32:48 PM CET Aleix Pol wrote:
> > On Wed, Dec 16, 2015 at 4:12 PM, Martin Graesslin 
> 
> wrote:
> > > Hi all,
> > > 
> > > following up on [1] I have prepared a split of frameworkintegration to
> > > move
> > > the QPT plugin into a dedicated repository. You can find it in [2].
> > > Please
> > > have a look at the split repo to verify that it looks fine. If
> > > everything
> > > is OK, I'll request a new repo from sysadmins.
> > > 
> > > Some general notes
> > > * new repo name: plasma-integration
> > > * new plugin name: PlasmaDesktopPlatformTheme
> > > * new src folder name: src/plasma-desktop-platformtheme
> > > 
> > > Explaining the name changes:
> > > The plugin is renamed to not conflict on install with the existing
> > > plugin
> > > from frameworkintegration and also incorporating future changes Marco
> > > pointed out: we need a different QPT plugin for mobile.
> > > 
> > > How to remove the plugin from frameworkintegration:
> > > After the split we cannot remove it from frameworkintegration yet as
> > > that
> > > would break setups on the next framework release. Given that I plan to
> > > only
> > > phase it out: Introduce a cmake option to build and install it, which
> > > defaults to true till at least Plasma 5.6 is released. Afterward
> > > swapping
> > > the default to false, with a big fat warning and keeping it like that
> > > for
> > > at least half a year. Then remove it. We don't provide any guarantees
> > > for
> > > it, so we can remove it, but I want to keep the impact small.
> > > 
> > > Cheers
> > > Martin
> > > 
> > > [1] https://mail.kde.org/pipermail/kde-frameworks-devel/2015-December/
> > > 029234.html
> > > [2] kde:scratch/graesslin/platform-integration
> > > (https://quickgit.kde.org/?
> > > p=scratch%2Fgraesslin%2Fplasma-integration.git )
> > 
> > I have my doubts that we want a different QPT for desktop and mobile.
> > If we want to provide any convergence we need to strive to have the
> > same on both platforms.
> 
> Good point! Marco is right: we need different presets whether it's desktop
> or mobile. You're right that different plugins would result in making
> switching impossible. Sounds like the plugin would have to learn to switch
> at runtime.
> 
> Given that feedback I'm considering to not rename the directory.

pushed a new variant without the rename of the source folder and the plugin 
renamed to KDEPlasmaPlatformTheme.

Cheers
Martin

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: about ki18n/locales: installing only a subset?

[Chusslove Illich]

>> [: Chusslove Illich :]
>> It has a use in every situation where multiple-language users work in a
>> centrally administered network. Like in... an office.
>
> [: René J.V. Bertin :]
> How many offices do you know that require all known languages to be
> installed? The only organisations I can think of where that situation
> might be approached are the UN and (to a much lesser extent) EU
> administrations.

I'm not talking about offices requiring it. I'm talking about coming to a
random office in the world and finding my language already available,
without trifling anyone about it.

>> To put it in perspective, a single contemporary high-budget game will
>> occupy much more disk blocks than all Gettext translations on the system
>> combined.
>
> You haven't read (or understood) my argument, which is about how blocks
> are used, not how many.

I read it, but apparently I didn't understand it. Would you explain it
further, or point me to an explanation?

-- 
Chusslove Illich (Часлав Илић)


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: Review Request 126392: Fix some Clazy warnings in kcoreaddons

[Emmanuel Pescosta]

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



src/lib/io/kdirwatch.cpp (line 1067)


This is wrong (if I'm not wrong ;)

De Morgan's law [1] says that A or B <=> not ((not A) and (not B))

Where in this case:
A, B = list.count 
(not A), (not B) = list.empty

[1] https://en.wikipedia.org/wiki/De_Morgan's_laws



src/lib/io/kdirwatch_p.h (line 109)


Same here


- Emmanuel Pescosta


On Dec. 18, 2015, 4:33 a.m., Andrey Cygankov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126392/
> ---
> 
> (Updated Dec. 18, 2015, 4:33 a.m.)
> 
> 
> Review request for KDE Frameworks and Aleix Pol Gonzalez.
> 
> 
> Repository: kcoreaddons
> 
> 
> Description
> ---
> 
> Fix some Clazy warnings:
> - unneeded heap allocation with QString
> - midRef() non-usage
> - isEmpty() non-usage
> 
> 
> Diffs
> -
> 
>   autotests/ktexttohtmltest.cpp 81c3fc0 
>   src/lib/io/kbackup.h ead2de5 
>   src/lib/io/kbackup.cpp ec8b8b2 
>   src/lib/io/kdirwatch.cpp 34b32b7 
>   src/lib/io/kdirwatch_p.h 4483961 
>   src/lib/io/kmessage.cpp 305318c 
>   src/lib/io/kprocess.cpp 1df61c0 
>   src/lib/io/kurlmimedata.cpp 4095ee1 
>   src/lib/kaboutdata.h 0dbd7a0 
>   src/lib/kaboutdata.cpp 5d9a55e 
>   src/lib/randomness/krandom.cpp 2eb80d2 
>   src/lib/text/kmacroexpander.cpp 0d05ffd 
>   src/lib/text/kstringhandler.cpp 826ddcb 
>   src/lib/text/ktexttohtml.cpp d58f5c0 
>   src/lib/util/kshell.cpp 11892ce 
> 
> Diff: https://git.reviewboard.kde.org/r/126392/diff/
> 
> 
> Testing
> ---
> 
> Compiling without errors
> 
> 
> Thanks,
> 
> Andrey Cygankov
> 
>

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

Re: Review Request 126392: Fix some Clazy warnings in kcoreaddons

[Aleix Pol Gonzalez]

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


+1 Looks good to me. Just checked and tests still pass.

- Aleix Pol Gonzalez


On Dec. 18, 2015, 4:33 a.m., Andrey Cygankov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126392/
> ---
> 
> (Updated Dec. 18, 2015, 4:33 a.m.)
> 
> 
> Review request for KDE Frameworks and Aleix Pol Gonzalez.
> 
> 
> Repository: kcoreaddons
> 
> 
> Description
> ---
> 
> Fix some Clazy warnings:
> - unneeded heap allocation with QString
> - midRef() non-usage
> - isEmpty() non-usage
> 
> 
> Diffs
> -
> 
>   autotests/ktexttohtmltest.cpp 81c3fc0 
>   src/lib/io/kbackup.h ead2de5 
>   src/lib/io/kbackup.cpp ec8b8b2 
>   src/lib/io/kdirwatch.cpp 34b32b7 
>   src/lib/io/kdirwatch_p.h 4483961 
>   src/lib/io/kmessage.cpp 305318c 
>   src/lib/io/kprocess.cpp 1df61c0 
>   src/lib/io/kurlmimedata.cpp 4095ee1 
>   src/lib/kaboutdata.h 0dbd7a0 
>   src/lib/kaboutdata.cpp 5d9a55e 
>   src/lib/randomness/krandom.cpp 2eb80d2 
>   src/lib/text/kmacroexpander.cpp 0d05ffd 
>   src/lib/text/kstringhandler.cpp 826ddcb 
>   src/lib/text/ktexttohtml.cpp d58f5c0 
>   src/lib/util/kshell.cpp 11892ce 
> 
> Diff: https://git.reviewboard.kde.org/r/126392/diff/
> 
> 
> Testing
> ---
> 
> Compiling without errors
> 
> 
> Thanks,
> 
> Andrey Cygankov
> 
>

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

Re: Review Request 126324: [MSWin/OS X] save and restore window geometry instead of only size (WIP/Suggestion)

[René J . V . Bertin]

> On Dec. 17, 2015, 5:16 p.m., Martin Gräßlin wrote:
> > src/gui/kwindowconfig.h, lines 38-39
> > 
> >
> > That doesn't match the method name. It's saveWindowSize, not 
> > saveWindowGeometry. It's highly unexpected that saveWindowSize saves the 
> > position.
> > 
> > If you want that: please introduce a new saveWindowGeometry method.
> 
> René J.V. Bertin wrote:
> I was afraid someone was going to say that, which is why I tried to argue 
> that it's highly unexpected from a user viewpoint that only window size is 
> saved and not position. How often would it happen that a developer is "highly 
> surprised" in a *negative* way that window size AND position are restored on 
> a platform where this is the default behaviour?
> 
> I have nothing against introducing a pair of new methods, but how is that 
> supposed to be done in transparent fashion? I do have a lot against a need to 
> change all dependent software to call those methods (maintenance burden and 
> all that).
> 
> Counter proposal: replace save/restoreWindowSize with 
> save/restoreWindowGeometry everywhere, with a platform-specific 
> interpretation of what exactly geometry encompasses. Much less surprise 
> there, just a bit more need to read the documentation. Are these functions 
> ever called intentionally outside of what I suppose is a more or less 
> automatic feature that takes care of restoring window, erm, layout (saving is 
> clearly automatic).
> 
> René J.V. Bertin wrote:
> Just to be clear: if I am going to introduce restore/saveWindowGeometry 
> methods they'll replace the WindowSize variants on OS X or at least those 
> will then use a different KConfig key to avoid conflicts. 
> I'd also be dropping the MS Windows part of the patch (as this is not a 
> decision I want to make for a platform I don't use).
> 
> But please consider this: that KConfig key has been called `geometry` for 
> a long time. Where exactly is the surprise, that restore/saveWindowSize never 
> did what the key they operate with suggests, or that they have always been 
> using an inaptly named key? For me the answer is straightforward and based on 
> what users will expect...
> 
> Martin Gräßlin wrote:
> I leave it to the maintainers. On API I maintain I would say no to 
> something changing the semantics like that.

As I just wrote in reply to a message from Valorie, I have absolutely no issue 
with maintaining methods for saving and restoring only window size, for code 
that somehow requires that. I'd guess that such code would probably enforce the 
intended window position itself *after* restoring window size (because that 
operation *can* affect window position), but in the end that's (indeed) up to 
the code's developers to decide.

IOW, I'm perfectly willing to discuss a better solution in which the burden to 
ensure that window save/restore works as "natively" as possible on each 
platform is shared. The best way to do that is of course to have a single pair 
of methods that have platform-specific implementations.

As far as I'm concerned such a solution might even be prepared completely in 
KConfig/gui before changes are made everywhere else to deploy that new 
solution. In that case I would for instance run temporary local (MacPorts) 
patches that replace saveWindowSize/restoreWindowSize with wrappers for 
saveWindowGeometry/restoreWindowGeometry.

Side-observation: OS X (Cocoa) provides a `[NSWindow setFrameAutosaveName:]` 
method, i.e. it avoids reference to specifics like size or geometry completely.

That method also provides another thought that could be taken into 
consideration if it is decided to evolve this part of the frameworks, something 
I'd be interested in collaborating on. Currently, there is no support for 
saving and restoring multiple windows per application. That may be more or less 
sufficient when applications always follow a MDI approach, but even if they do 
that still doesn't make them applications that are active only in a single 
instance. Example: KDevelop. One might expect that opening a given, 
pre-existing session (collection of open projects) restores the main window 
geometry (size and/or position) that used previously for that session, rather 
than the geometry used by whatever KDevelop session was run last. On OS X that 
would be done with something like `[NSWindow setFrameautosaveName:[window 
representedFile]]`, where `[NSWindow representedFile]` corresponds to 
`QWindow::filePath` (but AFAICS those are not coupled in Qt5).

I already had a quick look, but realised I don't know if the KConfig mechanism 
has facilities to handle cleanup of stale/obsolete key/value entries.


- René J.V.


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

Re: Review Request 126198: [OS X] adaptations for the KdePlatformTheme

[René J . V . Bertin]

> On Dec. 18, 2015, 8:11 a.m., Martin Gräßlin wrote:
> > I still want to see other OSX devs comment on that this is really the 
> > wanted approach. Also I need to point out that the split out of QPT to 
> > plasma-integration is prepared. I suggest to introduce a comparable 
> > osx-integration repository.

I have nothing against that, if done centrally. It could host/provide other 
things to, if set up appropriately. Maybe a better name would be inspired on 
Qt's MacExtras, X11Extras etc. (though arguably those are about improving 
integration).

KDE-Mac is CC'ed on each and every OS X related RR I file; from what I hear 
through the KDE-Mac ML their silence indicates general approval. You are of 
course not expected to take my word on this, but I do understand you have had 
some feedback from some of them concerning my contributions. And I think that 
includes the reality that I am currently basically the only active "OS X dev".

I'm aware that I'm in an uncomfortable position, working alone for and in name 
of a community the extent of which I don't even know. It's something that makes 
me wonder regularly why I'm doing it at all. I suppose that creating an 
osx-integration framework will make the lack of opinions from other "OSX devs" 
a bit more our internal affair rather than something affecting all of KF5?


> On Dec. 18, 2015, 8:11 a.m., Martin Gräßlin wrote:
> > src/platformtheme/kdeplatformtheme.cpp, lines 45-46
> > 
> >
> > Q_NULLPTR

I recall a relatively recent discussion on a Qt mailing list; IIUC Qt now 
advises to use `0` and not Q_NULLPTR anymore. It certainly makes the code more 
compact and (thus) readable; is there a reason KF5 sticks to Q_NULLPTR?


- René J.V.


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


On Dec. 17, 2015, 7:40 p.m., René J.V. Bertin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126198/
> ---
> 
> (Updated Dec. 17, 2015, 7:40 p.m.)
> 
> 
> Review request for KDE Software on Mac OS X, KDE Frameworks and Valorie 
> Zimmerman.
> 
> 
> Repository: frameworkintegration
> 
> 
> Description
> ---
> 
> The KdePlatformTheme plugin can be enabled on OS X with a minimal patch to 
> Qt5; all that is required is to include the `qgenericunixservices` and 
> `qgenericunixthemes` components in the build, and to append `"kde"` to the 
> list returned by `QCocoaIntegration::themeNames()` for instance under the 
> condition that `KDE_SESSION_VERSION` is set to a suitable value in the 
> environment.
> 
> This will allow KF5 and Qt5 applications to use the theme selected through 
> KDE if FrameworkIntegration is installed and KDE_SESSION_VERSION is set, 
> which seems like a sufficiently specific set of conditions that is easy to 
> avoid by users who prefer to use the Mac native theme.
> 
> While requestion the KDE theme is also possible through `-style kde` or `env 
> QT_STYLE_OVERRIDE=kde` the use of the KdePlatformTheme plugin appears to be 
> the only way to get the full theme, including the font and colour selection. 
> In my opinion it is above all the font customisation which is a very welcome 
> feature for Qt/Mac; by default Qt applications use the default system font 
> (Lucida Grande 13pt or even 14pt) throughout. This is a good UI font, but not 
> at that size (and most "native" OS X applications indeed use a range of 
> smaller sizes, depending on role.
> 
> It does have introduce a number of regressions, which the current patch aims 
> to address. The most visible and problematic of these regressions is the loss 
> of the Mac-style menu bar and thus of all menu items (actions).
> 
> The fix is straightforward : on OS X (and similarly affected platforms?), an 
> instance of the native Cocoa platform theme is created through the private 
> API, and used as a fallback rather than immediately falling back to the 
> default implementations from `QPlatformTheme`. In addition, methods missing 
> from (not overridden by) `KdePlatformTheme` are provided on OS X and call the 
> corresponding methods from the native theme. It is this change which restores 
> the menubar and even the Dock menu functionality.
> One minor regression remains but should be easy to fix (elsewhere?): the 
> Preferences menu loses its keyboard shortcut (Command-,).
> 
> Given the fallback nature of the native platform instance I have preferred to 
> print a warning rather than using something like `qFatal`, above all because 
> the message printed by qFatal tends to get lost on OS X. I can replace my use 
> of `qWarning` with a dialog giving the choice 

Re: Review Request 126198: [OS X] adaptations for the KdePlatformTheme

[Aleix Pol Gonzalez]

> On Dec. 18, 2015, 8:11 a.m., Martin Gräßlin wrote:
> > src/platformtheme/kdeplatformtheme.cpp, lines 45-46
> > 
> >
> > Q_NULLPTR
> 
> René J.V. Bertin wrote:
> I recall a relatively recent discussion on a Qt mailing list; IIUC Qt now 
> advises to use `0` and not Q_NULLPTR anymore. It certainly makes the code 
> more compact and (thus) readable; is there a reason KF5 sticks to Q_NULLPTR?

I don't see where you got that impression:
http://lists.qt-project.org/pipermail/development/2015-December/024040.html


- Aleix


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


On Dec. 17, 2015, 7:40 p.m., René J.V. Bertin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126198/
> ---
> 
> (Updated Dec. 17, 2015, 7:40 p.m.)
> 
> 
> Review request for KDE Software on Mac OS X, KDE Frameworks and Valorie 
> Zimmerman.
> 
> 
> Repository: frameworkintegration
> 
> 
> Description
> ---
> 
> The KdePlatformTheme plugin can be enabled on OS X with a minimal patch to 
> Qt5; all that is required is to include the `qgenericunixservices` and 
> `qgenericunixthemes` components in the build, and to append `"kde"` to the 
> list returned by `QCocoaIntegration::themeNames()` for instance under the 
> condition that `KDE_SESSION_VERSION` is set to a suitable value in the 
> environment.
> 
> This will allow KF5 and Qt5 applications to use the theme selected through 
> KDE if FrameworkIntegration is installed and KDE_SESSION_VERSION is set, 
> which seems like a sufficiently specific set of conditions that is easy to 
> avoid by users who prefer to use the Mac native theme.
> 
> While requestion the KDE theme is also possible through `-style kde` or `env 
> QT_STYLE_OVERRIDE=kde` the use of the KdePlatformTheme plugin appears to be 
> the only way to get the full theme, including the font and colour selection. 
> In my opinion it is above all the font customisation which is a very welcome 
> feature for Qt/Mac; by default Qt applications use the default system font 
> (Lucida Grande 13pt or even 14pt) throughout. This is a good UI font, but not 
> at that size (and most "native" OS X applications indeed use a range of 
> smaller sizes, depending on role.
> 
> It does have introduce a number of regressions, which the current patch aims 
> to address. The most visible and problematic of these regressions is the loss 
> of the Mac-style menu bar and thus of all menu items (actions).
> 
> The fix is straightforward : on OS X (and similarly affected platforms?), an 
> instance of the native Cocoa platform theme is created through the private 
> API, and used as a fallback rather than immediately falling back to the 
> default implementations from `QPlatformTheme`. In addition, methods missing 
> from (not overridden by) `KdePlatformTheme` are provided on OS X and call the 
> corresponding methods from the native theme. It is this change which restores 
> the menubar and even the Dock menu functionality.
> One minor regression remains but should be easy to fix (elsewhere?): the 
> Preferences menu loses its keyboard shortcut (Command-,).
> 
> Given the fallback nature of the native platform instance I have preferred to 
> print a warning rather than using something like `qFatal`, above all because 
> the message printed by qFatal tends to get lost on OS X. I can replace my use 
> of `qWarning` with a dialog giving the choice between continuing or exiting 
> the application - code that would be called in the menu methods because only 
> there is it certain that the application actually needs a menubar.
> 
> In line with experience and feedback from the KDE(4)-Mac community I have 
> decided to force the use of native dialogs rather than the ones from the 
> KdePlatformPlugin.
> 
> In addition I set the fallback value for `ShowIconsOnPushButtons` to false in 
> line with platform guidelines, and ensure that the autotests are not built as 
> app bundles.
> 
> 
> Diffs
> -
> 
>   src/platformtheme/kdemactheme.h PRE-CREATION 
>   src/platformtheme/kdemactheme.mm PRE-CREATION 
>   src/platformtheme/CMakeLists.txt 23f590e 
>   src/platformtheme/kdeplatformtheme.h 97d09df 
>   src/platformtheme/kdeplatformtheme.cpp 80dbcb7 
>   src/platformtheme/kfontsettingsdata.h 4b92c7d 
>   src/platformtheme/kfontsettingsdata.cpp b0a4bbf 
>   src/platformtheme/kfontsettingsdatamac.h PRE-CREATION 
>   src/platformtheme/kfontsettingsdatamac.mm PRE-CREATION 
>   src/platformtheme/khintssettings.h ec064d3 
>   src/platformtheme/khintssettings.cpp 8adf6c5 
>   src/platformtheme/khintssettingsmac.h 

Re: about ki18n/locales: installing only a subset?

[René J . V . Bertin]

On Friday December 18 2015 10:53:35 Chusslove Illich wrote:

>I'm not talking about offices requiring it. I'm talking about coming to a
>random office in the world and finding my language already available,
>without trifling anyone about it.

Out of curiosity: do you expect to use your language when communicating with 
people instead of computers, in any random office in the world? ;)

>> You haven't read (or understood) my argument, which is about how blocks
>> are used, not how many.
>
>I read it, but apparently I didn't understand it. Would you explain it
>further, or point me to an explanation?

In traditional disk/file system technology, files use an integer number of 
blocks, and blocks aren't shared among files. I am not aware that has changed 
with SSDs, but block sizes have increased. They used to be 512 bytes, which is 
a size that is small enough that the actual on-disk size for each file will 
never be more than 511 bytes larger than necessary. With 4096 byte blocks that 
potential waste grows to 4095 bytes, about 8x more.
Disclaimer: this is what *I* understand of the underlying functioning. I am not 
always right in my understanding of things.

As I said, I don't want to make a huge point of this. I'm perfectly aware that 
we're not in a situation like the last time I cleaned unwanted languages 
(.lproj directories) from OS X and freed up hundreds of Mbs from my boot 
partition. And in that case we were talking about absolute disk space waste :)

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

Re: Review Request 126198: [OS X] adaptations for the KdePlatformTheme

[René J . V . Bertin]

> On Dec. 18, 2015, 8:11 a.m., Martin Gräßlin wrote:
> > src/platformtheme/kdeplatformtheme.cpp, lines 45-46
> > 
> >
> > Q_NULLPTR
> 
> René J.V. Bertin wrote:
> I recall a relatively recent discussion on a Qt mailing list; IIUC Qt now 
> advises to use `0` and not Q_NULLPTR anymore. It certainly makes the code 
> more compact and (thus) readable; is there a reason KF5 sticks to Q_NULLPTR?
> 
> Aleix Pol Gonzalez wrote:
> I don't see where you got that impression:
> 
> http://lists.qt-project.org/pipermail/development/2015-December/024040.html

Is that your idea of *relatively* recent? :) I was thinking of a discussion a 
bit longer ago (but frankly, don't ask me to dig it up, I doubt I'd be able to).

I haven't read the full thread above, in fact I stopped reading at 
```
Arguments against:
- it's uglier than "0", and more to type
```

because that sums up exactly how I think about the question. But in the end I 
only care that there are no chances of 32bit/64bit conflicts (I had some of 
those in code of my own, which led me to be almost religious about using NULL 
for pointers).

I do have a bit of an issue with using a *Q_* prefix, as it suggests a 
Qt-specific type that should only be used in calls to Qt functions...


- René J.V.


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


On Dec. 17, 2015, 7:40 p.m., René J.V. Bertin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126198/
> ---
> 
> (Updated Dec. 17, 2015, 7:40 p.m.)
> 
> 
> Review request for KDE Software on Mac OS X, KDE Frameworks and Valorie 
> Zimmerman.
> 
> 
> Repository: frameworkintegration
> 
> 
> Description
> ---
> 
> The KdePlatformTheme plugin can be enabled on OS X with a minimal patch to 
> Qt5; all that is required is to include the `qgenericunixservices` and 
> `qgenericunixthemes` components in the build, and to append `"kde"` to the 
> list returned by `QCocoaIntegration::themeNames()` for instance under the 
> condition that `KDE_SESSION_VERSION` is set to a suitable value in the 
> environment.
> 
> This will allow KF5 and Qt5 applications to use the theme selected through 
> KDE if FrameworkIntegration is installed and KDE_SESSION_VERSION is set, 
> which seems like a sufficiently specific set of conditions that is easy to 
> avoid by users who prefer to use the Mac native theme.
> 
> While requestion the KDE theme is also possible through `-style kde` or `env 
> QT_STYLE_OVERRIDE=kde` the use of the KdePlatformTheme plugin appears to be 
> the only way to get the full theme, including the font and colour selection. 
> In my opinion it is above all the font customisation which is a very welcome 
> feature for Qt/Mac; by default Qt applications use the default system font 
> (Lucida Grande 13pt or even 14pt) throughout. This is a good UI font, but not 
> at that size (and most "native" OS X applications indeed use a range of 
> smaller sizes, depending on role.
> 
> It does have introduce a number of regressions, which the current patch aims 
> to address. The most visible and problematic of these regressions is the loss 
> of the Mac-style menu bar and thus of all menu items (actions).
> 
> The fix is straightforward : on OS X (and similarly affected platforms?), an 
> instance of the native Cocoa platform theme is created through the private 
> API, and used as a fallback rather than immediately falling back to the 
> default implementations from `QPlatformTheme`. In addition, methods missing 
> from (not overridden by) `KdePlatformTheme` are provided on OS X and call the 
> corresponding methods from the native theme. It is this change which restores 
> the menubar and even the Dock menu functionality.
> One minor regression remains but should be easy to fix (elsewhere?): the 
> Preferences menu loses its keyboard shortcut (Command-,).
> 
> Given the fallback nature of the native platform instance I have preferred to 
> print a warning rather than using something like `qFatal`, above all because 
> the message printed by qFatal tends to get lost on OS X. I can replace my use 
> of `qWarning` with a dialog giving the choice between continuing or exiting 
> the application - code that would be called in the menu methods because only 
> there is it certain that the application actually needs a menubar.
> 
> In line with experience and feedback from the KDE(4)-Mac community I have 
> decided to force the use of native dialogs rather than the ones from the 
> KdePlatformPlugin.
> 
> In addition I set the fallback value for `ShowIconsOnPushButtons` to false in 
> 

Review Request 126417: don't rely on char* to QString implicit conversion

[Patrick Spendrin]

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

Review request for KDE Frameworks and Plasma.


Repository: plasma-framework


Description
---

my Qt won't cast char* to QString, so explicitly use QStringLiteral here.


Diffs
-

  src/plasmaquick/appletquickitem.cpp 614b793fa724e7be0fb2dd8151374303fcc6ec18 

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


Testing
---

Windows.


Thanks,

Patrick Spendrin

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

Re: Review Request 126417: don't rely on char* to QString implicit conversion

[Martin Gräßlin]

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

Ship it!


Ship It!

- Martin Gräßlin


On Dec. 18, 2015, 3:20 p.m., Patrick Spendrin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126417/
> ---
> 
> (Updated Dec. 18, 2015, 3:20 p.m.)
> 
> 
> Review request for KDE Frameworks and Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> my Qt won't cast char* to QString, so explicitly use QStringLiteral here.
> 
> 
> Diffs
> -
> 
>   src/plasmaquick/appletquickitem.cpp 
> 614b793fa724e7be0fb2dd8151374303fcc6ec18 
> 
> Diff: https://git.reviewboard.kde.org/r/126417/diff/
> 
> 
> Testing
> ---
> 
> Windows.
> 
> 
> Thanks,
> 
> Patrick Spendrin
> 
>

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

Re: Review Request 126392: Fix some Clazy warnings in kcoreaddons

[Andrey Cygankov]

> On Дек. 18, 2015, 12:25 п.п., Emmanuel Pescosta wrote:
> > src/lib/io/kdirwatch.cpp, line 1067
> > 
> >
> > This is wrong (if I'm not wrong ;)
> > 
> > De Morgan's law [1] says that A or B <=> not ((not A) and (not B))
> > 
> > Where in this case:
> > A, B = list.count 
> > (not A), (not B) = list.empty
> > 
> > [1] https://en.wikipedia.org/wiki/De_Morgan's_laws

Yes , I realized the error , correct it soon.


- Andrey


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


On Дек. 18, 2015, 3:33 д.п., Andrey Cygankov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126392/
> ---
> 
> (Updated Дек. 18, 2015, 3:33 д.п.)
> 
> 
> Review request for KDE Frameworks and Aleix Pol Gonzalez.
> 
> 
> Repository: kcoreaddons
> 
> 
> Description
> ---
> 
> Fix some Clazy warnings:
> - unneeded heap allocation with QString
> - midRef() non-usage
> - isEmpty() non-usage
> 
> 
> Diffs
> -
> 
>   autotests/ktexttohtmltest.cpp 81c3fc0 
>   src/lib/io/kbackup.h ead2de5 
>   src/lib/io/kbackup.cpp ec8b8b2 
>   src/lib/io/kdirwatch.cpp 34b32b7 
>   src/lib/io/kdirwatch_p.h 4483961 
>   src/lib/io/kmessage.cpp 305318c 
>   src/lib/io/kprocess.cpp 1df61c0 
>   src/lib/io/kurlmimedata.cpp 4095ee1 
>   src/lib/kaboutdata.h 0dbd7a0 
>   src/lib/kaboutdata.cpp 5d9a55e 
>   src/lib/randomness/krandom.cpp 2eb80d2 
>   src/lib/text/kmacroexpander.cpp 0d05ffd 
>   src/lib/text/kstringhandler.cpp 826ddcb 
>   src/lib/text/ktexttohtml.cpp d58f5c0 
>   src/lib/util/kshell.cpp 11892ce 
> 
> Diff: https://git.reviewboard.kde.org/r/126392/diff/
> 
> 
> Testing
> ---
> 
> Compiling without errors
> 
> 
> Thanks,
> 
> Andrey Cygankov
> 
>

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

Re: Review Request 126327: sonnet: Do not use QStringLiteral with multi strings

[Patrick Spendrin]

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

(Updated Dec. 18, 2015, 1:32 p.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks and Martin Tobias Holmedahl Sandsmark.


Changes
---

Submitted with commit 738ccd36971aadbd9fe85d09e4087c64a2a604ad by Patrick R. 
Spendrin to branch master.


Repository: sonnet


Description
---

Strings that are separated into multiple parts don't work on Windows
together with QStringLiteral as the first string is interpreted as a
wide (16bit) string, and the second one as a narrow (8bit) string.
Replacing with QString::fromLatin1 is the easiest solution keeping
the code layout the same, joining the strings does work too though.


Diffs
-

  examples/dialogexample.cpp 4bda1ad4c2e8df1d51e7007838fbc43747919304 
  examples/plaintextedit.cpp 45b5503612cbdb32796228bcee1b53a472e9d313 
  examples/textedit.cpp a1ccb28aab35e2491b1512241a05767429ce84d9 

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


Testing
---

Windows.


Thanks,

Patrick Spendrin

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

Re: Review Request 126417: don't rely on char* to QString implicit conversion

[Kai Uwe Broulik]

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


i'm wondering whether those are still needed

- Kai Uwe Broulik


On Dez. 18, 2015, 2:20 nachm., Patrick Spendrin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126417/
> ---
> 
> (Updated Dez. 18, 2015, 2:20 nachm.)
> 
> 
> Review request for KDE Frameworks and Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> my Qt won't cast char* to QString, so explicitly use QStringLiteral here.
> 
> 
> Diffs
> -
> 
>   src/plasmaquick/appletquickitem.cpp 
> 614b793fa724e7be0fb2dd8151374303fcc6ec18 
> 
> Diff: https://git.reviewboard.kde.org/r/126417/diff/
> 
> 
> Testing
> ---
> 
> Windows.
> 
> 
> Thanks,
> 
> Patrick Spendrin
> 
>

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

Re: Review Request 126416: MetaFunction doesn't need to be exported

[Aleix Pol Gonzalez]

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


Maybe it would be best to move the implementation to a .cpp file?

- Aleix Pol Gonzalez


On Dec. 18, 2015, 3:05 p.m., Patrick Spendrin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126416/
> ---
> 
> (Updated Dec. 18, 2015, 3:05 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kross
> 
> 
> Description
> ---
> 
> Header only functions don't need to be exported, as there is no code in
> the dynamic library available.
> 
> 
> Diffs
> -
> 
>   src/core/metafunction.h 71ce3470ca627f4d2b90b5cdd94cfe87a41c9a8f 
> 
> Diff: https://git.reviewboard.kde.org/r/126416/diff/
> 
> 
> Testing
> ---
> 
> Windows.
> 
> 
> Thanks,
> 
> Patrick Spendrin
> 
>

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

Re: Review Request 126417: don't rely on char* to QString implicit conversion

[Aleix Pol Gonzalez]

> On Dec. 18, 2015, 3:35 p.m., Kai Uwe Broulik wrote:
> > i'm wondering whether those are still needed

Well, it's still being called.

+1 on the patch, it can be further fixed later.


- Aleix


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


On Dec. 18, 2015, 3:20 p.m., Patrick Spendrin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126417/
> ---
> 
> (Updated Dec. 18, 2015, 3:20 p.m.)
> 
> 
> Review request for KDE Frameworks and Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> my Qt won't cast char* to QString, so explicitly use QStringLiteral here.
> 
> 
> Diffs
> -
> 
>   src/plasmaquick/appletquickitem.cpp 
> 614b793fa724e7be0fb2dd8151374303fcc6ec18 
> 
> Diff: https://git.reviewboard.kde.org/r/126417/diff/
> 
> 
> Testing
> ---
> 
> Windows.
> 
> 
> Thanks,
> 
> Patrick Spendrin
> 
>

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

Re: Review Request 126198: [OS X] adaptations for the KdePlatformTheme

[René J . V . Bertin]

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

(Updated Dec. 18, 2015, 5:53 p.m.)


Review request for KDE Software on Mac OS X, KDE Frameworks and Valorie 
Zimmerman.


Changes
---

replaced 0 with Q_NULLPTR where appropriate, in the changed and new code I'm 
proposing.


Repository: frameworkintegration


Description
---

The KdePlatformTheme plugin can be enabled on OS X with a minimal patch to Qt5; 
all that is required is to include the `qgenericunixservices` and 
`qgenericunixthemes` components in the build, and to append `"kde"` to the list 
returned by `QCocoaIntegration::themeNames()` for instance under the condition 
that `KDE_SESSION_VERSION` is set to a suitable value in the environment.

This will allow KF5 and Qt5 applications to use the theme selected through KDE 
if FrameworkIntegration is installed and KDE_SESSION_VERSION is set, which 
seems like a sufficiently specific set of conditions that is easy to avoid by 
users who prefer to use the Mac native theme.

While requestion the KDE theme is also possible through `-style kde` or `env 
QT_STYLE_OVERRIDE=kde` the use of the KdePlatformTheme plugin appears to be the 
only way to get the full theme, including the font and colour selection. In my 
opinion it is above all the font customisation which is a very welcome feature 
for Qt/Mac; by default Qt applications use the default system font (Lucida 
Grande 13pt or even 14pt) throughout. This is a good UI font, but not at that 
size (and most "native" OS X applications indeed use a range of smaller sizes, 
depending on role.

It does have introduce a number of regressions, which the current patch aims to 
address. The most visible and problematic of these regressions is the loss of 
the Mac-style menu bar and thus of all menu items (actions).

The fix is straightforward : on OS X (and similarly affected platforms?), an 
instance of the native Cocoa platform theme is created through the private API, 
and used as a fallback rather than immediately falling back to the default 
implementations from `QPlatformTheme`. In addition, methods missing from (not 
overridden by) `KdePlatformTheme` are provided on OS X and call the 
corresponding methods from the native theme. It is this change which restores 
the menubar and even the Dock menu functionality.
One minor regression remains but should be easy to fix (elsewhere?): the 
Preferences menu loses its keyboard shortcut (Command-,).

Given the fallback nature of the native platform instance I have preferred to 
print a warning rather than using something like `qFatal`, above all because 
the message printed by qFatal tends to get lost on OS X. I can replace my use 
of `qWarning` with a dialog giving the choice between continuing or exiting the 
application - code that would be called in the menu methods because only there 
is it certain that the application actually needs a menubar.

In line with experience and feedback from the KDE(4)-Mac community I have 
decided to force the use of native dialogs rather than the ones from the 
KdePlatformPlugin.

In addition I set the fallback value for `ShowIconsOnPushButtons` to false in 
line with platform guidelines, and ensure that the autotests are not built as 
app bundles.


Diffs (updated)
-

  src/platformtheme/CMakeLists.txt 23f590e 
  src/platformtheme/kdemactheme.h PRE-CREATION 
  src/platformtheme/kdemactheme.mm PRE-CREATION 
  src/platformtheme/kdeplatformtheme.h 97d09df 
  src/platformtheme/kdeplatformtheme.cpp 80dbcb7 
  src/platformtheme/kfontsettingsdata.h 4b92c7d 
  src/platformtheme/kfontsettingsdata.cpp b0a4bbf 
  src/platformtheme/kfontsettingsdatamac.h PRE-CREATION 
  src/platformtheme/kfontsettingsdatamac.mm PRE-CREATION 
  src/platformtheme/khintssettings.h ec064d3 
  src/platformtheme/khintssettings.cpp 8adf6c5 
  src/platformtheme/khintssettingsmac.h PRE-CREATION 
  src/platformtheme/khintssettingsmac.mm PRE-CREATION 
  src/platformtheme/main_mac.cpp PRE-CREATION 

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


Testing
---

On Mac OS X with Qt 5.5.1, KF5 frameworks 5.16.0 and QtCurve git/head.

I have not verified to what extent my use of a private `QGuiApplication` API 
links builds to a specific Qt version (I consider that nothing shocking and a 
minor price to pay).
>>> Do I need to add some glue to the CMake file so that it will warn if the 
>>> private headerfiles are not available? Apparently no changes were required 
>>> to find them.


File Attachments


purely native OS X theme
  
https://git.reviewboard.kde.org/media/uploaded/files/2015/11/30/650d0da7-52b3-40d1-a1f9-cb610494cf77__Screen_Shot_2015-11-30_at_15.42.31.png
native theme but with `-style kde`
  

Re: Review Request 126392: Fix some Clazy warnings in kcoreaddons

[Andrey Cygankov]

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

(Updated Дек. 18, 2015, 4:57 п.п.)


Review request for KDE Frameworks and Aleix Pol Gonzalez.


Repository: kcoreaddons


Description
---

Fix some Clazy warnings:
- unneeded heap allocation with QString
- midRef() non-usage
- isEmpty() non-usage


Diffs (updated)
-

  autotests/ktexttohtmltest.cpp 81c3fc0 
  src/lib/io/kbackup.h ead2de5 
  src/lib/io/kbackup.cpp ec8b8b2 
  src/lib/io/kdirwatch.cpp 34b32b7 
  src/lib/io/kdirwatch_p.h 4483961 
  src/lib/io/kmessage.cpp 305318c 
  src/lib/io/kprocess.cpp 1df61c0 
  src/lib/io/kurlmimedata.cpp 4095ee1 
  src/lib/kaboutdata.h 0dbd7a0 
  src/lib/kaboutdata.cpp 5d9a55e 
  src/lib/randomness/krandom.cpp 2eb80d2 
  src/lib/text/kmacroexpander.cpp 0d05ffd 
  src/lib/text/kstringhandler.cpp 826ddcb 
  src/lib/text/ktexttohtml.cpp d58f5c0 
  src/lib/util/kshell.cpp 11892ce 

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


Testing
---

Compiling without errors


Thanks,

Andrey Cygankov

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

Re: Review Request 126392: Fix some Clazy warnings in kcoreaddons

[Aleix Pol Gonzalez]

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



src/lib/io/kdirwatch_p.h (line 109)


That's still wrong. it should be:
`return !m_clients.isEmpty() || !m_entries.isEmpty();`


- Aleix Pol Gonzalez


On Dec. 18, 2015, 5:57 p.m., Andrey Cygankov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126392/
> ---
> 
> (Updated Dec. 18, 2015, 5:57 p.m.)
> 
> 
> Review request for KDE Frameworks and Aleix Pol Gonzalez.
> 
> 
> Repository: kcoreaddons
> 
> 
> Description
> ---
> 
> Fix some Clazy warnings:
> - unneeded heap allocation with QString
> - midRef() non-usage
> - isEmpty() non-usage
> 
> 
> Diffs
> -
> 
>   autotests/ktexttohtmltest.cpp 81c3fc0 
>   src/lib/io/kbackup.h ead2de5 
>   src/lib/io/kbackup.cpp ec8b8b2 
>   src/lib/io/kdirwatch.cpp 34b32b7 
>   src/lib/io/kdirwatch_p.h 4483961 
>   src/lib/io/kmessage.cpp 305318c 
>   src/lib/io/kprocess.cpp 1df61c0 
>   src/lib/io/kurlmimedata.cpp 4095ee1 
>   src/lib/kaboutdata.h 0dbd7a0 
>   src/lib/kaboutdata.cpp 5d9a55e 
>   src/lib/randomness/krandom.cpp 2eb80d2 
>   src/lib/text/kmacroexpander.cpp 0d05ffd 
>   src/lib/text/kstringhandler.cpp 826ddcb 
>   src/lib/text/ktexttohtml.cpp d58f5c0 
>   src/lib/util/kshell.cpp 11892ce 
> 
> Diff: https://git.reviewboard.kde.org/r/126392/diff/
> 
> 
> Testing
> ---
> 
> Compiling without errors
> 
> 
> Thanks,
> 
> Andrey Cygankov
> 
>

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

Re: Review Request 126418: Add KWin/Decoration support to plasmapkg

[Martin Gräßlin]

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


Looks good to me.

- Martin Gräßlin


On Dec. 18, 2015, 7:30 p.m., Demitrius Belai wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126418/
> ---
> 
> (Updated Dec. 18, 2015, 7:30 p.m.)
> 
> 
> Review request for KDE Frameworks and Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> Hi,
> 
> This is same #105677 but for plasmapkg2
> 
> 
> Diffs
> -
> 
>   src/plasmapkg/plasmapkg.cpp 4626323cd5d1dc3a3147fe43102d901d615e0b66 
> 
> Diff: https://git.reviewboard.kde.org/r/126418/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Demitrius Belai
> 
>

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

Review Request 126418: Add KWin/Decoration support to plasmapkg

[Demitrius Belai]

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

Review request for KDE Frameworks and Plasma.


Summary (updated)
-

Add KWin/Decoration support to plasmapkg


Repository: plasma-framework


Description
---

Hi,

This is same #105677 but for plasmapkg2


Diffs (updated)
-

  src/plasmapkg/plasmapkg.cpp 4626323cd5d1dc3a3147fe43102d901d615e0b66 

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


Testing
---


Thanks,

Demitrius Belai

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

Re: Review Request 126417: don't rely on char* to QString implicit conversion

[Patrick Spendrin]

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

(Updated Dec. 18, 2015, 11:29 p.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks and Plasma.


Changes
---

Submitted with commit 54736e262f80b62686c11d542d5d239233243d1f by Patrick 
Spendrin to branch master.


Repository: plasma-framework


Description
---

my Qt won't cast char* to QString, so explicitly use QStringLiteral here.


Diffs
-

  src/plasmaquick/appletquickitem.cpp 614b793fa724e7be0fb2dd8151374303fcc6ec18 

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


Testing
---

Windows.


Thanks,

Patrick Spendrin

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

Re: Review Request 126369: [OS X] adaptation(s) to platform limitations (WIP)

[René J . V . Bertin]

> On Dec. 18, 2015, 11:27 p.m., Martin Klapetek wrote:
> > src/kstatusnotifieritem.cpp, lines 279-281
> > 
> >
> > This has a strange indentation
> > 
> > Also I think those bools are not entirely useful,
> > it just makes the code harder to read by using
> > shortened names while you could just plug those
> > title.isEmpty()/subTitle.isEmpty() directly in
> > those ifs. Each is going to be called exactly
> > once anyway.

Ah, yes indeed (the indentation). Not sure who did it, but I missed it. And 
you're right about the bools.


> On Dec. 18, 2015, 11:27 p.m., Martin Klapetek wrote:
> > src/kstatusnotifieritem.cpp, line 908
> > 
> >
> > This is missing an ifdef

Will be fixed in the next upload, tomorrow morning if life doesn't get too much 
in the way.


> On Dec. 18, 2015, 11:27 p.m., Martin Klapetek wrote:
> > src/kstatusnotifieritem.cpp, line 909
> > 
> >
> > I really think this should be handled/mapped
> > by the OS X native systray thing. But I guess
> > there is no API that can be used/mapped to the
> > NeedsAttention state?

I agree, but no, I haven't seen anything in the APIs that can be used to 
request attention via the systray icon. I'll have another look, but for now the 
only native attention mechanism that I can think of is letting the Dock icon 
bounce. That'll work only for applications that have a Dock icon of course, and 
it tends to work only when the application is in the background. So badging the 
systray icon is probably still going to be a good idea, and I doubt that it's 
going to take less code to do that with native APIs.

It also requires using ObjC. What do you think of the idea of moving 
`KStatusNotifierItemPrivate` to its own file, and subclassing it on OS X to 
override the `init` and `syncLegacySystemTrayIcon` methods? I can put that into 
an ObjC++ file which means I can do native API calls. I think a few of the 
other modifications I made to the main class could also be moved into 
`KStatusNotifierItemPrivate` (and overriden in `KStatusNotifierItemPrivateMac`) 
to reduce the number of ifdefs.


- René J.V.


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


On Dec. 17, 2015, 5:26 p.m., René J.V. Bertin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126369/
> ---
> 
> (Updated Dec. 17, 2015, 5:26 p.m.)
> 
> 
> Review request for KDE Software on Mac OS X and KDE Frameworks.
> 
> 
> Repository: knotifications
> 
> 
> Description
> ---
> 
> OS X has a number of limitations in features used by KNotifications, notably 
> concerning the status notifier item (aka system tray icon).
> 
> This RR will likely evolve to address multiple limitations (at least also the 
> NeedsAttention state); at the moment it only proposes an emulation of 
> `QMenu::addSection`.
> 
> `QMenu::addSection` works by adding a QAction with a "texted separator" at 
> the insertion location. Texted separators do not exist in menu items in the 
> OS X "global" menubar (they become regular separators), and Qt will not 
> provide a platform-specific implementation. Loss of the section title text is 
> maybe not always an issue, but I think it is in the system tray menu. I 
> therefore propose to emulate `QMenu::addSection` by replacing the texted 
> separator with an inactive (disabled) menu item that shows the text, followed 
> by a standard separator. Menus in the notification area are much less subject 
> to interface guidelines, so the presence of an item icon is acceptable and 
> IMO useful for the `titleAction`.
> 
> Testing the NeedsAttention state with the tests/kstatusnotifieritemtest 
> application leads to disappearance of the menubar icon, i.e. the access to 
> the notifier menu becomes invisible rather than blinking (which is what I get 
> on Linux using the same packaging). Adding a few qDebug statements shows that 
> the `attentionIcon` is empty.
> I'd appreciate a crash course how this feature is supposed to work, so I can 
> see if an OS X implementation might be feasible.
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt 6d09051 
>   src/CMakeLists.txt 7eb3125 
>   src/kstatusnotifieritem.cpp f9bf460 
>   tests/kstatusnotifieritemtest.cpp 973fc85 
> 
> Diff: https://git.reviewboard.kde.org/r/126369/diff/
> 
> 
> Testing
> ---
> 
> On OS X 10.9.5 with Qt 5.5.1 and frameworks 5.17.0 .
> 
> 
> File Attachments
> 
> 
> the 

Review Request 126416: MetaFunction doesn't need to be exported

[Patrick Spendrin]

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

Review request for KDE Frameworks.


Repository: kross


Description
---

Header only functions don't need to be exported, as there is no code in
the dynamic library available.


Diffs
-

  src/core/metafunction.h 71ce3470ca627f4d2b90b5cdd94cfe87a41c9a8f 

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


Testing
---

Windows.


Thanks,

Patrick Spendrin

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

Re: Review Request 126198: [OS X] adaptations for the KdePlatformTheme

[Martin Gräßlin]

> On Dec. 18, 2015, 8:11 a.m., Martin Gräßlin wrote:
> > src/platformtheme/kdeplatformtheme.cpp, lines 45-46
> > 
> >
> > Q_NULLPTR
> 
> René J.V. Bertin wrote:
> I recall a relatively recent discussion on a Qt mailing list; IIUC Qt now 
> advises to use `0` and not Q_NULLPTR anymore. It certainly makes the code 
> more compact and (thus) readable; is there a reason KF5 sticks to Q_NULLPTR?
> 
> Aleix Pol Gonzalez wrote:
> I don't see where you got that impression:
> 
> http://lists.qt-project.org/pipermail/development/2015-December/024040.html
> 
> René J.V. Bertin wrote:
> Is that your idea of *relatively* recent? :) I was thinking of a 
> discussion a bit longer ago (but frankly, don't ask me to dig it up, I doubt 
> I'd be able to).
> 
> I haven't read the full thread above, in fact I stopped reading at 
> ```
> Arguments against:
> - it's uglier than "0", and more to type
> ```
> 
> because that sums up exactly how I think about the question. But in the 
> end I only care that there are no chances of 32bit/64bit conflicts (I had 
> some of those in code of my own, which led me to be almost religious about 
> using NULL for pointers).
> 
> I do have a bit of an issue with using a *Q_* prefix, as it suggests a 
> Qt-specific type that should only be used in calls to Qt functions...

Rene, could you please not turn every review suggestion into a long discussion? 
And could you please remove your personal preferences. I also don't like to use 
Q_NULLPTR and would prefer nullptr, but that's not allowed in frameworks. Given 
that Q_NULLPTR is the best we have. It's better than 0 or NULL.


- Martin


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


On Dec. 17, 2015, 7:40 p.m., René J.V. Bertin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126198/
> ---
> 
> (Updated Dec. 17, 2015, 7:40 p.m.)
> 
> 
> Review request for KDE Software on Mac OS X, KDE Frameworks and Valorie 
> Zimmerman.
> 
> 
> Repository: frameworkintegration
> 
> 
> Description
> ---
> 
> The KdePlatformTheme plugin can be enabled on OS X with a minimal patch to 
> Qt5; all that is required is to include the `qgenericunixservices` and 
> `qgenericunixthemes` components in the build, and to append `"kde"` to the 
> list returned by `QCocoaIntegration::themeNames()` for instance under the 
> condition that `KDE_SESSION_VERSION` is set to a suitable value in the 
> environment.
> 
> This will allow KF5 and Qt5 applications to use the theme selected through 
> KDE if FrameworkIntegration is installed and KDE_SESSION_VERSION is set, 
> which seems like a sufficiently specific set of conditions that is easy to 
> avoid by users who prefer to use the Mac native theme.
> 
> While requestion the KDE theme is also possible through `-style kde` or `env 
> QT_STYLE_OVERRIDE=kde` the use of the KdePlatformTheme plugin appears to be 
> the only way to get the full theme, including the font and colour selection. 
> In my opinion it is above all the font customisation which is a very welcome 
> feature for Qt/Mac; by default Qt applications use the default system font 
> (Lucida Grande 13pt or even 14pt) throughout. This is a good UI font, but not 
> at that size (and most "native" OS X applications indeed use a range of 
> smaller sizes, depending on role.
> 
> It does have introduce a number of regressions, which the current patch aims 
> to address. The most visible and problematic of these regressions is the loss 
> of the Mac-style menu bar and thus of all menu items (actions).
> 
> The fix is straightforward : on OS X (and similarly affected platforms?), an 
> instance of the native Cocoa platform theme is created through the private 
> API, and used as a fallback rather than immediately falling back to the 
> default implementations from `QPlatformTheme`. In addition, methods missing 
> from (not overridden by) `KdePlatformTheme` are provided on OS X and call the 
> corresponding methods from the native theme. It is this change which restores 
> the menubar and even the Dock menu functionality.
> One minor regression remains but should be easy to fix (elsewhere?): the 
> Preferences menu loses its keyboard shortcut (Command-,).
> 
> Given the fallback nature of the native platform instance I have preferred to 
> print a warning rather than using something like `qFatal`, above all because 
> the message printed by qFatal tends to get lost on OS X. I can replace my use 
> of `qWarning` with a dialog giving the choice between continuing or exiting 

Re: Review Request 126410: Fix FILE* leak in kwalletd

[Martin Klapetek]

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

Ship it!


Ship It!

- Martin Klapetek


On Dec. 18, 2015, 2:57 a.m., Michael Pyne wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126410/
> ---
> 
> (Updated Dec. 18, 2015, 2:57 a.m.)
> 
> 
> Review request for KDE Frameworks, Martin Klapetek and Valentin Rusu.
> 
> 
> Repository: kwallet
> 
> 
> Description
> ---
> 
> Fix a leak of a `FILE*` in kwalletd, spotted by Coverity (CID 1335090), and a 
> couple of minor fixes/cleanups since I was there.
> 
> 1. No need to memset a `char[]`, you can let compiler default-initialize it 
> (and likely even be able to move the whole array into .bss).
> 2. The chop after `fgets` can easily be confused for a null terminator check, 
> and `fgets` isn't required to return a newline anyways, so add sanity 
> checking (which helps document better at the same time).
> 
> 
> Diffs
> -
> 
>   src/runtime/kwalletd/main.cpp db8d55c 
> 
> Diff: https://git.reviewboard.kde.org/r/126410/diff/
> 
> 
> Testing
> ---
> 
> Everything compiles. Not much beyond done.
> 
> 
> 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 126369: [OS X] adaptation(s) to platform limitations (WIP)

[Martin Klapetek]

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



src/kstatusnotifieritem.cpp (lines 279 - 281)


This has a strange indentation

Also I think those bools are not entirely useful,
it just makes the code harder to read by using
shortened names while you could just plug those
title.isEmpty()/subTitle.isEmpty() directly in
those ifs. Each is going to be called exactly
once anyway.



src/kstatusnotifieritem.cpp (line 908)


This is missing an ifdef



src/kstatusnotifieritem.cpp (line 909)


I really think this should be handled/mapped
by the OS X native systray thing. But I guess
there is no API that can be used/mapped to the
NeedsAttention state?


- Martin Klapetek


On Dec. 17, 2015, 5:26 p.m., René J.V. Bertin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126369/
> ---
> 
> (Updated Dec. 17, 2015, 5:26 p.m.)
> 
> 
> Review request for KDE Software on Mac OS X and KDE Frameworks.
> 
> 
> Repository: knotifications
> 
> 
> Description
> ---
> 
> OS X has a number of limitations in features used by KNotifications, notably 
> concerning the status notifier item (aka system tray icon).
> 
> This RR will likely evolve to address multiple limitations (at least also the 
> NeedsAttention state); at the moment it only proposes an emulation of 
> `QMenu::addSection`.
> 
> `QMenu::addSection` works by adding a QAction with a "texted separator" at 
> the insertion location. Texted separators do not exist in menu items in the 
> OS X "global" menubar (they become regular separators), and Qt will not 
> provide a platform-specific implementation. Loss of the section title text is 
> maybe not always an issue, but I think it is in the system tray menu. I 
> therefore propose to emulate `QMenu::addSection` by replacing the texted 
> separator with an inactive (disabled) menu item that shows the text, followed 
> by a standard separator. Menus in the notification area are much less subject 
> to interface guidelines, so the presence of an item icon is acceptable and 
> IMO useful for the `titleAction`.
> 
> Testing the NeedsAttention state with the tests/kstatusnotifieritemtest 
> application leads to disappearance of the menubar icon, i.e. the access to 
> the notifier menu becomes invisible rather than blinking (which is what I get 
> on Linux using the same packaging). Adding a few qDebug statements shows that 
> the `attentionIcon` is empty.
> I'd appreciate a crash course how this feature is supposed to work, so I can 
> see if an OS X implementation might be feasible.
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt 6d09051 
>   src/CMakeLists.txt 7eb3125 
>   src/kstatusnotifieritem.cpp f9bf460 
>   tests/kstatusnotifieritemtest.cpp 973fc85 
> 
> Diff: https://git.reviewboard.kde.org/r/126369/diff/
> 
> 
> Testing
> ---
> 
> On OS X 10.9.5 with Qt 5.5.1 and frameworks 5.17.0 .
> 
> 
> File Attachments
> 
> 
> the systray icon & menu created by kstatusnotifieritemtest . This application 
> has no icon to show.
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2015/12/16/286037ae-07b3-454a-a226-1748854493a1__kstatusnotifieritemtest-systray.png
> The systray icon and menu created by the KDE4 kwalletmanager (code has an 
> equivalent patch)
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2015/12/16/4fc9d4e4-1537-478c-9196-94cbc17b6b7c__kwalletmanager-systray.png
> An Apple systray icon+menu that shows icons (which cannot be hidden)
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2015/12/16/fc48a963-2e18-4396-bd38-062d41688118__Apple-systray-menu-with-icons.png
> kstatusnotifieritemtest with added appIcon
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2015/12/17/e896e90d-f0a8-43f7-9199-847572832df7__kstatusnotifieritemtest-with-appIcon.png
> kstatusnotifieritemtest-with-appIcon+attention.png
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2015/12/17/f41081db-8e09-4ea2-95c1-f507c62109d4__kstatusnotifieritemtest-with-appIconattention.png
> 
> 
> Thanks,
> 
> René J.V. Bertin
> 
>

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

Re: Review Request 126410: Fix FILE* leak in kwalletd

[Michael Pyne]

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

(Updated Dec. 18, 2015, 10:50 p.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks, Martin Klapetek and Valentin Rusu.


Changes
---

Submitted with commit 1d0bec904dd78ff21d1d68e9c3e363ad1a77f7ed by Michael Pyne 
to branch master.


Repository: kwallet


Description
---

Fix a leak of a `FILE*` in kwalletd, spotted by Coverity (CID 1335090), and a 
couple of minor fixes/cleanups since I was there.

1. No need to memset a `char[]`, you can let compiler default-initialize it 
(and likely even be able to move the whole array into .bss).
2. The chop after `fgets` can easily be confused for a null terminator check, 
and `fgets` isn't required to return a newline anyways, so add sanity checking 
(which helps document better at the same time).


Diffs
-

  src/runtime/kwalletd/main.cpp db8d55c 

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


Testing
---

Everything compiles. Not much beyond done.


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 126369: [OS X] adaptation(s) to platform limitations (WIP)

[Martin Klapetek]

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


> is it to be expected (in kstatusnotifieritemtest.cpp) that tray->iconPixmap() 
> returns a null QIcon immediately after calling tray->setIconByName()

I believe so, it's "either-or", either you set the icon by name or
you set it by pixmap. If you set it by name, you don't really need
to call the iconPixmap I think because you can easily do that on any
local QIcon.

This is also somewhat supported by the spec at [1].

> how exactly is the NeedsAttention mechanism supposed to work?

This is fully up to the host system iirc. Plasma5 does not swap the
icons anymore, there is a whole different effect which just zoomes
the normal icon. In other words, the needsAttention icon is not used
at all. OS X should adopt whichever mechanism is most "home" to it.


[1] http://www.notmart.org/misc/statusnotifieritem/statusnotifieritem.html

- Martin Klapetek


On Dec. 17, 2015, 5:26 p.m., René J.V. Bertin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126369/
> ---
> 
> (Updated Dec. 17, 2015, 5:26 p.m.)
> 
> 
> Review request for KDE Software on Mac OS X and KDE Frameworks.
> 
> 
> Repository: knotifications
> 
> 
> Description
> ---
> 
> OS X has a number of limitations in features used by KNotifications, notably 
> concerning the status notifier item (aka system tray icon).
> 
> This RR will likely evolve to address multiple limitations (at least also the 
> NeedsAttention state); at the moment it only proposes an emulation of 
> `QMenu::addSection`.
> 
> `QMenu::addSection` works by adding a QAction with a "texted separator" at 
> the insertion location. Texted separators do not exist in menu items in the 
> OS X "global" menubar (they become regular separators), and Qt will not 
> provide a platform-specific implementation. Loss of the section title text is 
> maybe not always an issue, but I think it is in the system tray menu. I 
> therefore propose to emulate `QMenu::addSection` by replacing the texted 
> separator with an inactive (disabled) menu item that shows the text, followed 
> by a standard separator. Menus in the notification area are much less subject 
> to interface guidelines, so the presence of an item icon is acceptable and 
> IMO useful for the `titleAction`.
> 
> Testing the NeedsAttention state with the tests/kstatusnotifieritemtest 
> application leads to disappearance of the menubar icon, i.e. the access to 
> the notifier menu becomes invisible rather than blinking (which is what I get 
> on Linux using the same packaging). Adding a few qDebug statements shows that 
> the `attentionIcon` is empty.
> I'd appreciate a crash course how this feature is supposed to work, so I can 
> see if an OS X implementation might be feasible.
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt 6d09051 
>   src/CMakeLists.txt 7eb3125 
>   src/kstatusnotifieritem.cpp f9bf460 
>   tests/kstatusnotifieritemtest.cpp 973fc85 
> 
> Diff: https://git.reviewboard.kde.org/r/126369/diff/
> 
> 
> Testing
> ---
> 
> On OS X 10.9.5 with Qt 5.5.1 and frameworks 5.17.0 .
> 
> 
> File Attachments
> 
> 
> the systray icon & menu created by kstatusnotifieritemtest . This application 
> has no icon to show.
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2015/12/16/286037ae-07b3-454a-a226-1748854493a1__kstatusnotifieritemtest-systray.png
> The systray icon and menu created by the KDE4 kwalletmanager (code has an 
> equivalent patch)
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2015/12/16/4fc9d4e4-1537-478c-9196-94cbc17b6b7c__kwalletmanager-systray.png
> An Apple systray icon+menu that shows icons (which cannot be hidden)
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2015/12/16/fc48a963-2e18-4396-bd38-062d41688118__Apple-systray-menu-with-icons.png
> kstatusnotifieritemtest with added appIcon
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2015/12/17/e896e90d-f0a8-43f7-9199-847572832df7__kstatusnotifieritemtest-with-appIcon.png
> kstatusnotifieritemtest-with-appIcon+attention.png
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2015/12/17/f41081db-8e09-4ea2-95c1-f507c62109d4__kstatusnotifieritemtest-with-appIconattention.png
> 
> 
> Thanks,
> 
> René J.V. Bertin
> 
>

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