D28651: Load and use global animation settings

[Martin Tobias Holmedahl Sandsmark]

sandsmark added a comment.


  Should I move this to invent, or just push it?

REPOSITORY
  R31 Breeze

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

To: sandsmark, #breeze
Cc: cblack, davidedmundson, ngraham, hpereiradacosta, ndavis, plasma-devel, 
#breeze, Orage, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, 
GB_2, trickyricky26, ragreen, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27034: Fix message box when passing a remote url that includes a file

[Martin Tobias Holmedahl Sandsmark]

sandsmark added a comment.


  This is wrong, isn't it? The logic was: if setDirectory is passed something 
that _isn't_ a directory, try to strip the filename.

REPOSITORY
  R135 Integration for Qt applications in Plasma

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

To: apol, #frameworks, ngraham
Cc: sandsmark, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29808: KSysGuard Network Plugin: Don't use std::regex to parse the network files.

[Martin Tobias Holmedahl Sandsmark]

sandsmark added inline comments.

INLINE COMMENTS

> sandsmark wrote in ConnectionMapping.cpp:167
> instead of 87 + 1, maybe have a `constexpr int lineLength = strlen("0: 
> 0100:0277 : 
> 0A : 00:  00 16741");` above with 
> the comment?
> or:
> 
>   constexpr const char *exampleLine = "0: 
> 0100:0277 : 
> 0A : 00:  00 16741";
>   constexpr int lineLength = strlen(exampleLine);
> 
> both more understandable and less prone to accidentally putting in the wrong 
> number.
> 
> same with all other magic string offset numbers here.

To see that it is actually optimized properly: https://godbolt.org/z/sL2_pK

REPOSITORY
  R106 KSysguard

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

To: dkorth, ahiemstra
Cc: sandsmark, ahiemstra, jpalecek, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, 
ngraham, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, mart

D29808: KSysGuard Network Plugin: Don't use std::regex to parse the network files.

[Martin Tobias Holmedahl Sandsmark]

sandsmark added inline comments.

INLINE COMMENTS

> ConnectionMapping.cpp:157
> +// Should be within the first 16 characters.
> +size_t data_start = data.find(':');
> +if (data_start >= 16) {

if we're going for ultra optimized, const everything I guess.

> ConnectionMapping.cpp:167
> +// Check for IPv4.
> +if (data.size() < data_start + 87 + 1) {
> +continue;

instead of 87 + 1, maybe have a `constexpr int lineLength = strlen("0: 
0100:0277 : 0A 
: 00:  00 16741");` above with the 
comment?
or:

  constexpr const char *exampleLine = "0: 0100:0277 
: 0A : 00:  
00 16741";
  constexpr int lineLength = strlen(exampleLine);

both more understandable and less prone to accidentally putting in the wrong 
number.

same with all other magic string offset numbers here.

> ConnectionMapping.cpp:176
> +char *const ipv4 = [data_start + 2];
> +ipv4[8] = '\0';
> +localAddress.address[3] = (uint32_t)strtoul(ipv4, nullptr, 16);

strtoul stops at the first non-digit, so is this really necessary?

> ConnectionMapping.cpp:203
> +ipv6[32] = '\0';
> +localAddress.address[3] = (uint32_t)strtoul([24], 
> nullptr, 16);
> +} else {

I prefer the more c++-ish cast of `uint32_t(foo);` (I think I got that from the 
Qt code style guidelines).

REPOSITORY
  R106 KSysguard

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

To: dkorth, ahiemstra
Cc: sandsmark, ahiemstra, jpalecek, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, 
ngraham, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, mart

D29808: KSysGuard Network Plugin: Don't use std::regex to parse the network files.

[Martin Tobias Holmedahl Sandsmark]

sandsmark added a comment.


  > It's all C code whereas the rest of the helper is C++. It also relies very 
heavily on magic numbers now.
  
  
  
  > I think a much simpler implementation would be to split each line on " ", 
select the fields we want and clean them up.
  
  I assume this is for performance reasons, but a tiny microbenchmark showing 
that it is actually faster would be nice.

REPOSITORY
  R106 KSysguard

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

To: dkorth, ahiemstra
Cc: sandsmark, ahiemstra, jpalecek, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, 
ngraham, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, mart

D28651: Load and use global animation settings

[Martin Tobias Holmedahl Sandsmark]

sandsmark updated this revision to Diff 80551.
sandsmark added a comment.


  Remove the duplication of animation control, and don't override the animation 
settings if people haven't adjusted it globally.

REPOSITORY
  R31 Breeze

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28651?vs=79916=80551

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

AFFECTED FILES
  kdecoration/config/breezeconfigwidget.cpp
  kstyle/breezestyle.cpp
  kstyle/breezestyle.h
  kstyle/config/breezestyleconfig.cpp
  kstyle/config/ui/breezestyleconfig.ui

To: sandsmark, #breeze
Cc: davidedmundson, ngraham, hpereiradacosta, ndavis, plasma-devel, #breeze, 
Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, 
GB_2, trickyricky26, ragreen, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28651: Load and use global animation settings

[Martin Tobias Holmedahl Sandsmark]

sandsmark added a comment.


  In D28651#647122 , @davidedmundson 
wrote:
  
  > As for runtime changes I'm trying to migrate more things to KConfigWatcher 
which I wrote to replace random ad-hoc ints everywhere as well as making sure 
we automatically reparse the config once and only once.
  >
  > It's going to be /amazing/ but it's being rolled out as a slow migration, 
so there's nothing wrong with merging this as-is and migrating later.
  
  
  It looks kind of amazing, and it would get rid of that annoying magic `3` I 
think?

REPOSITORY
  R31 Breeze

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

To: sandsmark, #breeze
Cc: davidedmundson, ngraham, hpereiradacosta, ndavis, plasma-devel, #breeze, 
Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, 
GB_2, trickyricky26, ragreen, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28651: Load and use global animation settings

[Martin Tobias Holmedahl Sandsmark]

sandsmark added a comment.


  In D28651#646572 , @ndavis wrote:
  
  > You need to fix the git author info. If you upload a patch via the web UI 
instead of `arc`, the author info gets messed up.
  
  
  I usually just push normally after approval, but I'll see what I can do.
  
  > It's still working for me. I also still need to restart apps for changes to 
the global animation settings to apply.
  
  Could you run `dbus-monitor` while changing it, and look for the 
`org.kde.Breeze.Style` and `org.kde.KGlobalSettings` messages? And are the 
other changes made in the UI updated automatically (which should be reloaded by 
the `org.kde.Breeze.Style`)?

REPOSITORY
  R31 Breeze

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

To: sandsmark, #breeze
Cc: ngraham, hpereiradacosta, ndavis, plasma-devel, #breeze, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, trickyricky26, 
ragreen, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, ahiemstra, mart

D28651: Load and use global animation settings

[Martin Tobias Holmedahl Sandsmark]

sandsmark updated this revision to Diff 79916.
sandsmark added a comment.


  Now should reload the animation settings when changed anywhere.

REPOSITORY
  R31 Breeze

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28651?vs=79914=79916

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

AFFECTED FILES
  kdecoration/config/breezeconfigwidget.cpp
  kstyle/breezestyle.cpp
  kstyle/breezestyle.h

To: sandsmark, #breeze
Cc: ngraham, hpereiradacosta, ndavis, plasma-devel, #breeze, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, trickyricky26, 
ragreen, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, ahiemstra, mart

D28651: Load and use global animation settings

[Martin Tobias Holmedahl Sandsmark]

sandsmark added a comment.


  In D28651#643681 , @ndavis wrote:
  
  > I don't know enough about KDE configuration management to judge the code, 
but with this patch, changing animation speeds in SySe works if I restart apps 
after the change.
  
  
  The breeze plugin needs to do something like this: 
https://cgit.kde.org/plasma-integration.git/tree/src/platformtheme/khintssettings.cpp#n234
 (and now I see that the slot there is missing some stuff, I'll see if I can 
fix that later).

REPOSITORY
  R31 Breeze

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

To: sandsmark, #breeze
Cc: ngraham, hpereiradacosta, ndavis, plasma-devel, #breeze, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, trickyricky26, 
ragreen, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, ahiemstra, mart

D28651: Load and use global animation settings

[Martin Tobias Holmedahl Sandsmark]

sandsmark updated this revision to Diff 79914.
sandsmark added a comment.


  Also made it store to the global configuration. This way it is backwards 
compatible, but the config can also be changed from both places.
  
  I think it makes sense to have it both places, having it in the breeze 
settings avoids people having to go back and forth to tune their settings.

REPOSITORY
  R31 Breeze

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28651?vs=79566=79914

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

AFFECTED FILES
  kdecoration/config/breezeconfigwidget.cpp
  kstyle/breezestyle.cpp

To: sandsmark, #breeze
Cc: ngraham, hpereiradacosta, ndavis, plasma-devel, #breeze, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, trickyricky26, 
ragreen, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, ahiemstra, mart

D28651: Load and use global animation settings

[Martin Tobias Holmedahl Sandsmark]

sandsmark created this revision.
sandsmark added a reviewer: Breeze.
sandsmark added a project: Breeze.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
sandsmark requested review of this revision.

REVISION SUMMARY
  In addition to the specific Breeze animation settings, KDE has "global" 
animation settings primarily used for `Qt::UIEffect`s like 
`Qt::UI_AnimateMenu`, `Qt::UI_AnimateCombo`, `Qt::UI_AnimateTooltip` and 
`Qt::UI_AnimateToolBox`.
  
  This patch ensures that Breeze use and respect those settings, which both 
harmonizes with other styles (if `QGuiApplication::desktopSettingsAware()` is 
true).

TEST PLAN
  Turn animations on and off in kdeglobals, and see animations turn off and on 
when using Breeze.
  
  Also makes https://phabricator.kde.org/D17732 work properly with breeze.

REPOSITORY
  R31 Breeze

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

AFFECTED FILES
  kstyle/breezestyle.cpp

To: sandsmark, #breeze
Cc: plasma-devel, #breeze, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, trickyricky26, ragreen, ZrenBot, ngraham, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

Re: Review Request 128429: Fix strings in tooltips

[Martin Tobias Holmedahl Sandsmark]

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

(Updated Feb. 3, 2017, 8:12 p.m.)


Status
--

This change has been marked as submitted.


Review request for Plasma, Albert Astals Cid, Chusslove Illich, and John 
Tapsell.


Changes
---

Submitted with commit 6c5f127cb3825802ac504be94995f7bcd94da1f5 by Martin T. H. 
Sandsmark to branch master.


Repository: libksysguard


Description
---

Changed the strings to use proper kuit markup.


Diffs
-

  processui/ProcessModel.cpp 8151dfe 

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


Testing
---

Viewed all the tooltips.


Thanks,

Martin Tobias Holmedahl Sandsmark

Re: Review Request 128429: Fix strings in tooltips

[Martin Tobias Holmedahl Sandsmark]

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

(Updated Jan. 29, 2017, 1:38 p.m.)


Review request for Plasma, Albert Astals Cid, Chusslove Illich, and John 
Tapsell.


Changes
---

rebased on master, as well as; ping?


Repository: libksysguard


Description
---

Changed the strings to use proper kuit markup.


Diffs (updated)
-

  processui/ProcessModel.cpp 8151dfe 

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


Testing
---

Viewed all the tooltips.


Thanks,

Martin Tobias Holmedahl Sandsmark

Re: Review Request 128761: Fix crash on exit

[Martin Tobias Holmedahl Sandsmark]

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



Calling exit() before deleting the QApplication is explicitly not supported by 
Qt, it will lead to random crashes all over the place. See e. g. 
https://quickgit.kde.org/?p=konsole.git=commit=fe334292b5402ad0fd4b934291160ece9a12d953
 and https://bugreports.qt.io/browse/QTBUG-48709

- Martin Tobias Holmedahl Sandsmark


On Aug. 27, 2016, 9:12 a.m., Peter Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128761/
> ---
> 
> (Updated Aug. 27, 2016, 9:12 a.m.)
> 
> 
> Review request for Plasma, David Edmundson, David Faure, and Hugo Pereira Da 
> Costa.
> 
> 
> Bugs: 356940
> https://bugs.kde.org/show_bug.cgi?id=356940
> 
> 
> Repository: oxygen
> 
> 
> Description
> ---
> 
> Since Qt 5.6.0, Qt5 applications started crashing on exit. All signs
> point to this delete-on-destroy hack which was added to avoid outliving
> the plugin lifetime.
> 
> This method is wrong because the returned style is owned by the caller
> (QApplication, QProxyStyle, etc) and will cleaned up when those users
> are destructed.
> 
> Copied from breeze patch https://git.reviewboard.kde.org/r/128760/
> 
> 
> Diffs
> -
> 
>   kstyle/oxygenstyleplugin.cpp 70b90d9 
> 
> Diff: https://git.reviewboard.kde.org/r/128761/diff/
> 
> 
> Testing
> ---
> 
> Started `QT_STYLE_OVERRIDE=oxygen LD_LIBRARY_PATH=... QT_PLUGIN_PATH=... 
> wireshark -o` (an invalid option that triggers `exit(1)`) and observe a 
> heap-use-after free similar to the one reported in the bug. Apply this patch, 
> rebuild oxygen and notice that the crash is fixed. Also tested with "Testcase 
> (ASAN)" from bug 356940, crash is also gone.
> 
> 
> Thanks,
> 
> Peter Wu
> 
>

Re: Review Request 128473: Avoid recursive calls to QPlatformTheme::createPlatformSystemTrayIcon()

[Martin Tobias Holmedahl Sandsmark]

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

(Updated Aug. 13, 2016, 7:31 p.m.)


Status
--

This change has been discarded.


Review request for Plasma.


Repository: plasma-integration


Description
---

If the status notifier item host is not available, KSNI tries to create a 
normal QSystemTrayIcon.

The plasma platform plugin uses KSNI when it is called to create a 
QPlatformSystemTrayIcon.

So if the status notifier item host for any reason was unavailable, this would 
recursively run forever (assuming a turing machine with infinite memory).


Diffs
-

  src/platformtheme/kdeplatformsystemtrayicon.h 6825b4d 
  src/platformtheme/kdeplatformsystemtrayicon.cpp 0e82385 
  src/platformtheme/kdeplatformtheme.cpp 5f0407c 

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


Testing
---

Now it is possible to run applications that have tray icons with the plasma 
platform plugin even when the status notifier item host is down or unavailable.


Thanks,

Martin Tobias Holmedahl Sandsmark

Re: Review Request 128473: Avoid recursive calls to QPlatformTheme::createPlatformSystemTrayIcon()

[Martin Tobias Holmedahl Sandsmark]

> On July 19, 2016, 4:02 a.m., David Edmundson wrote:
> > KStatusNotifierItemPrivate::setLegacySystemTrayEnabled(bool enabled)
> > already has a recursion check added in 
> > b45544f3d4dd9cb1873b92a609f45a68f5f6e471  (in knotifications) - which 
> > basically checks if we're using the KDE platform theme (albeit in a 
> > slightly weird way)
> > 
> > Not saying yours is "worse" but we don't want two fixes in two places.
> > 
> > Could you check you have that patch? and why it doesn't work?
> 
> Martin Tobias Holmedahl Sandsmark wrote:
> firstly, as you said, it checks in a weird way, which doesn't work, 
> that's why I thought it made more sense to fix it in the platform theme 
> itself which already knows that it is loaded and whether an SNI host is 
> available.
> 
> (fwiw, qApp->platformName() is not correct either, that's what I thought 
> was the "proper" way to do it)
> 
> Martin Tobias Holmedahl Sandsmark wrote:
> patching around that also leads to no legacy tray icon being created at 
> all, which is obviously wrong.
> 
> David Edmundson wrote:
> Is it wrong?
> 
> If you're logged in to a plasma session with the plasma integration 
> running it's a valid assumption that people will run Plasmashell. 
> 
> Without plasmashell you won't get the legacy tray icons appearing either. 
> 
> And if you're running a different shell..you shouldn't be using the 
> plasma integration in the first place.
> 
> Martin Tobias Holmedahl Sandsmark wrote:
> The legacy tray icons will work in cases where SNI won't, so breaking 
> that fallback is wrong in my opinion, but I agree it is better than the 
> alternative.
> 
> Anyhow, this is a mess and the interdependencies on behaviour is bad. 
> IMHO it should be "fixed" in both places, the plasma-integration plugin 
> shouldn't rely on some shaky string magic logic in KSNI not to hang 
> applications.
> 
> So if the only objection to this patch is that some applications a) under 
> X11 get a legacy tray icon instead of a SNI one if started too early instead 
> of hanging, b) under Wayland might not get a tray icon at all instead of just 
> hanging if started too early, I think this should go in.
> 
> David Edmundson wrote:
> >The legacy tray icons will work in cases where SNI won't
> 
> How? If Plasmashell is running you get the SNIs. If plasmashell is not 
> running you don't get the legacy icons anyway.
> 
> It's not going in until you can explain:
> 1) why this is needed
> 2) why the other patch doesn't work.
> 
> Martin Tobias Holmedahl Sandsmark wrote:
> > How? If Plasmashell is running you get the SNIs. If plasmashell is not 
> running you don't get the legacy icons anyway.
> 
> Because they're very different technologies with very different kinds of 
> bugs? There's a reason the fallback is there already.
> 
> and that's just the unintended ways it can fail, then you have users that 
> for some reasons intentionally don't run plasma-shell with the default 
> settings, use another tray solution, etc.
> 
> 
> > 2) why the other patch doesn't work.
> 
> The other patch won't work because it doesn't know if the plasma 
> integration plugin is loaded. it's also the wrong place to put those kinds of 
> checks, even if you would find a 100% bulletproof bug free way to detect 
> that. hardcoding in fixes for bugs in platform plugins (e. g. if another 
> platform plugin decides to do the same) is wrong.
> 
> and as already demonstrated, it doesn't work well in practice.
> 
> 
> > 1) why this is needed
> 
> to avoid applications hanging/crashing, approach the zen of failing 
> gracefully, and in general make this a bit more robust.
> 
> imho, this shouldn't even be using the same KSNI classes that 
> applications are supposed to use, it's a design error that leads to these 
> kinds of problems, but this makes it a bit more sane.
> 
> 
> but this discussion was absurd several posts ago, and it's not getting 
> better...
> 
> Martin Gräßlin wrote:
> So could you please explain why the existing solution doesn't work and 
> why this is needed in addition? We just try to understand and whether there 
> are things we need to change in KNotifications.
> 
> Btw. the fact that this will break SNIs during startup in a racy way on 
> Wayland is a reason for me to not make it go in. We need to start thinking 
> about the "can break in Wayland" cases more, after all there are people using 
> it productivly

Re: Review Request 128491: Give more useful names to the acpi thermal zones and cooling devices

[Martin Tobias Holmedahl Sandsmark]

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

(Updated Aug. 13, 2016, 10:17 a.m.)


Status
--

This change has been marked as submitted.


Review request for Plasma and John Tapsell.


Changes
---

Submitted with commit db794732898379d1a05adf858871f9c999e9886b by Martin T. H. 
Sandsmark to branch master.


Repository: ksysguard


Description
---

Also fix the fact that cooling devices have a percentage of activity, they're 
not boolean on or off.

I'll reverse the tabs->spaces later, if needed.


Diffs
-

  ksysguardd/Linux/acpi.h e50b281 
  ksysguardd/Linux/acpi.c acf54e1 

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


Testing
---

It works very well.


Thanks,

Martin Tobias Holmedahl Sandsmark

Re: Review Request 128429: Fix strings in tooltips

[Martin Tobias Holmedahl Sandsmark]

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

(Updated Aug. 6, 2016, 1:26 p.m.)


Review request for Plasma, Albert Astals Cid, Chusslove Illich, and John 
Tapsell.


Changes
---

fix missing closing tag


Repository: libksysguard


Description
---

Changed the strings to use proper kuit markup.


Diffs (updated)
-

  processui/ProcessModel.cpp c55cea6 

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


Testing
---

Viewed all the tooltips.


Thanks,

Martin Tobias Holmedahl Sandsmark

Re: Review Request 128473: Avoid recursive calls to QPlatformTheme::createPlatformSystemTrayIcon()

[Martin Tobias Holmedahl Sandsmark]

> On July 19, 2016, 4:02 a.m., David Edmundson wrote:
> > KStatusNotifierItemPrivate::setLegacySystemTrayEnabled(bool enabled)
> > already has a recursion check added in 
> > b45544f3d4dd9cb1873b92a609f45a68f5f6e471  (in knotifications) - which 
> > basically checks if we're using the KDE platform theme (albeit in a 
> > slightly weird way)
> > 
> > Not saying yours is "worse" but we don't want two fixes in two places.
> > 
> > Could you check you have that patch? and why it doesn't work?
> 
> Martin Tobias Holmedahl Sandsmark wrote:
> firstly, as you said, it checks in a weird way, which doesn't work, 
> that's why I thought it made more sense to fix it in the platform theme 
> itself which already knows that it is loaded and whether an SNI host is 
> available.
> 
> (fwiw, qApp->platformName() is not correct either, that's what I thought 
> was the "proper" way to do it)
> 
> Martin Tobias Holmedahl Sandsmark wrote:
> patching around that also leads to no legacy tray icon being created at 
> all, which is obviously wrong.
> 
> David Edmundson wrote:
> Is it wrong?
> 
> If you're logged in to a plasma session with the plasma integration 
> running it's a valid assumption that people will run Plasmashell. 
> 
> Without plasmashell you won't get the legacy tray icons appearing either. 
> 
> And if you're running a different shell..you shouldn't be using the 
> plasma integration in the first place.
> 
> Martin Tobias Holmedahl Sandsmark wrote:
> The legacy tray icons will work in cases where SNI won't, so breaking 
> that fallback is wrong in my opinion, but I agree it is better than the 
> alternative.
> 
> Anyhow, this is a mess and the interdependencies on behaviour is bad. 
> IMHO it should be "fixed" in both places, the plasma-integration plugin 
> shouldn't rely on some shaky string magic logic in KSNI not to hang 
> applications.
> 
> So if the only objection to this patch is that some applications a) under 
> X11 get a legacy tray icon instead of a SNI one if started too early instead 
> of hanging, b) under Wayland might not get a tray icon at all instead of just 
> hanging if started too early, I think this should go in.
> 
> David Edmundson wrote:
> >The legacy tray icons will work in cases where SNI won't
> 
> How? If Plasmashell is running you get the SNIs. If plasmashell is not 
> running you don't get the legacy icons anyway.
> 
> It's not going in until you can explain:
> 1) why this is needed
> 2) why the other patch doesn't work.
> 
> Martin Tobias Holmedahl Sandsmark wrote:
> > How? If Plasmashell is running you get the SNIs. If plasmashell is not 
> running you don't get the legacy icons anyway.
> 
> Because they're very different technologies with very different kinds of 
> bugs? There's a reason the fallback is there already.
> 
> and that's just the unintended ways it can fail, then you have users that 
> for some reasons intentionally don't run plasma-shell with the default 
> settings, use another tray solution, etc.
> 
> 
> > 2) why the other patch doesn't work.
> 
> The other patch won't work because it doesn't know if the plasma 
> integration plugin is loaded. it's also the wrong place to put those kinds of 
> checks, even if you would find a 100% bulletproof bug free way to detect 
> that. hardcoding in fixes for bugs in platform plugins (e. g. if another 
> platform plugin decides to do the same) is wrong.
> 
> and as already demonstrated, it doesn't work well in practice.
> 
> 
> > 1) why this is needed
> 
> to avoid applications hanging/crashing, approach the zen of failing 
> gracefully, and in general make this a bit more robust.
> 
> imho, this shouldn't even be using the same KSNI classes that 
> applications are supposed to use, it's a design error that leads to these 
> kinds of problems, but this makes it a bit more sane.
> 
> 
> but this discussion was absurd several posts ago, and it's not getting 
> better...
> 
> Martin Gräßlin wrote:
> So could you please explain why the existing solution doesn't work and 
> why this is needed in addition? We just try to understand and whether there 
> are things we need to change in KNotifications.
> 
> Btw. the fact that this will break SNIs during startup in a racy way on 
> Wayland is a reason for me to not make it go in. We need to start thinking 
> about the "can break in Wayland" cases more, after all there are people using 
> it producti

Re: Review Request 128491: Give more useful names to the acpi thermal zones and cooling devices

[Martin Tobias Holmedahl Sandsmark]

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



unless there's any objections I'm going to commit this soonish.

- Martin Tobias Holmedahl Sandsmark


On July 21, 2016, 2:25 p.m., Martin Tobias Holmedahl Sandsmark wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128491/
> ---
> 
> (Updated July 21, 2016, 2:25 p.m.)
> 
> 
> Review request for Plasma and John Tapsell.
> 
> 
> Repository: ksysguard
> 
> 
> Description
> ---
> 
> Also fix the fact that cooling devices have a percentage of activity, they're 
> not boolean on or off.
> 
> I'll reverse the tabs->spaces later, if needed.
> 
> 
> Diffs
> -
> 
>   ksysguardd/Linux/acpi.h e50b281 
>   ksysguardd/Linux/acpi.c acf54e1 
> 
> Diff: https://git.reviewboard.kde.org/r/128491/diff/
> 
> 
> Testing
> ---
> 
> It works very well.
> 
> 
> Thanks,
> 
> Martin Tobias Holmedahl Sandsmark
> 
>

Re: Review Request 128473: Avoid recursive calls to QPlatformTheme::createPlatformSystemTrayIcon()

[Martin Tobias Holmedahl Sandsmark]

> On July 19, 2016, 4:02 a.m., David Edmundson wrote:
> > KStatusNotifierItemPrivate::setLegacySystemTrayEnabled(bool enabled)
> > already has a recursion check added in 
> > b45544f3d4dd9cb1873b92a609f45a68f5f6e471  (in knotifications) - which 
> > basically checks if we're using the KDE platform theme (albeit in a 
> > slightly weird way)
> > 
> > Not saying yours is "worse" but we don't want two fixes in two places.
> > 
> > Could you check you have that patch? and why it doesn't work?
> 
> Martin Tobias Holmedahl Sandsmark wrote:
> firstly, as you said, it checks in a weird way, which doesn't work, 
> that's why I thought it made more sense to fix it in the platform theme 
> itself which already knows that it is loaded and whether an SNI host is 
> available.
> 
> (fwiw, qApp->platformName() is not correct either, that's what I thought 
> was the "proper" way to do it)
> 
> Martin Tobias Holmedahl Sandsmark wrote:
> patching around that also leads to no legacy tray icon being created at 
> all, which is obviously wrong.
> 
> David Edmundson wrote:
> Is it wrong?
> 
> If you're logged in to a plasma session with the plasma integration 
> running it's a valid assumption that people will run Plasmashell. 
> 
> Without plasmashell you won't get the legacy tray icons appearing either. 
> 
> And if you're running a different shell..you shouldn't be using the 
> plasma integration in the first place.
> 
> Martin Tobias Holmedahl Sandsmark wrote:
> The legacy tray icons will work in cases where SNI won't, so breaking 
> that fallback is wrong in my opinion, but I agree it is better than the 
> alternative.
> 
> Anyhow, this is a mess and the interdependencies on behaviour is bad. 
> IMHO it should be "fixed" in both places, the plasma-integration plugin 
> shouldn't rely on some shaky string magic logic in KSNI not to hang 
> applications.
> 
> So if the only objection to this patch is that some applications a) under 
> X11 get a legacy tray icon instead of a SNI one if started too early instead 
> of hanging, b) under Wayland might not get a tray icon at all instead of just 
> hanging if started too early, I think this should go in.
> 
> David Edmundson wrote:
> >The legacy tray icons will work in cases where SNI won't
> 
> How? If Plasmashell is running you get the SNIs. If plasmashell is not 
> running you don't get the legacy icons anyway.
> 
> It's not going in until you can explain:
> 1) why this is needed
> 2) why the other patch doesn't work.

> How? If Plasmashell is running you get the SNIs. If plasmashell is not 
> running you don't get the legacy icons anyway.

Because they're very different technologies with very different kinds of bugs? 
There's a reason the fallback is there already.

and that's just the unintended ways it can fail, then you have users that for 
some reasons intentionally don't run plasma-shell with the default settings, 
use another tray solution, etc.


> 2) why the other patch doesn't work.

The other patch won't work because it doesn't know if the plasma integration 
plugin is loaded. it's also the wrong place to put those kinds of checks, even 
if you would find a 100% bulletproof bug free way to detect that. hardcoding in 
fixes for bugs in platform plugins (e. g. if another platform plugin decides to 
do the same) is wrong.

and as already demonstrated, it doesn't work well in practice.


> 1) why this is needed

to avoid applications hanging/crashing, approach the zen of failing gracefully, 
and in general make this a bit more robust.

imho, this shouldn't even be using the same KSNI classes that applications are 
supposed to use, it's a design error that leads to these kinds of problems, but 
this makes it a bit more sane.


but this discussion was absurd several posts ago, and it's not getting better...


- Martin Tobias Holmedahl


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


On July 17, 2016, 8:14 p.m., Martin Tobias Holmedahl Sandsmark wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128473/
> ---
> 
> (Updated July 17, 2016, 8:14 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Repository: plasma-integration
> 
> 
> Description
> ---
> 
> If the status notifier item host is not available, KS

Re: Review Request 128473: Avoid recursive calls to QPlatformTheme::createPlatformSystemTrayIcon()

[Martin Tobias Holmedahl Sandsmark]

> On July 19, 2016, 4:02 a.m., David Edmundson wrote:
> > KStatusNotifierItemPrivate::setLegacySystemTrayEnabled(bool enabled)
> > already has a recursion check added in 
> > b45544f3d4dd9cb1873b92a609f45a68f5f6e471  (in knotifications) - which 
> > basically checks if we're using the KDE platform theme (albeit in a 
> > slightly weird way)
> > 
> > Not saying yours is "worse" but we don't want two fixes in two places.
> > 
> > Could you check you have that patch? and why it doesn't work?
> 
> Martin Tobias Holmedahl Sandsmark wrote:
> firstly, as you said, it checks in a weird way, which doesn't work, 
> that's why I thought it made more sense to fix it in the platform theme 
> itself which already knows that it is loaded and whether an SNI host is 
> available.
> 
> (fwiw, qApp->platformName() is not correct either, that's what I thought 
> was the "proper" way to do it)
> 
> Martin Tobias Holmedahl Sandsmark wrote:
> patching around that also leads to no legacy tray icon being created at 
> all, which is obviously wrong.
> 
> David Edmundson wrote:
> Is it wrong?
> 
> If you're logged in to a plasma session with the plasma integration 
> running it's a valid assumption that people will run Plasmashell. 
> 
> Without plasmashell you won't get the legacy tray icons appearing either. 
> 
> And if you're running a different shell..you shouldn't be using the 
> plasma integration in the first place.

The legacy tray icons will work in cases where SNI won't, so breaking that 
fallback is wrong in my opinion, but I agree it is better than the alternative.

Anyhow, this is a mess and the interdependencies on behaviour is bad. IMHO it 
should be "fixed" in both places, the plasma-integration plugin shouldn't rely 
on some shaky string magic logic in KSNI not to hang applications.

So if the only objection to this patch is that some applications a) under X11 
get a legacy tray icon instead of a SNI one if started too early instead of 
hanging, b) under Wayland might not get a tray icon at all instead of just 
hanging if started too early, I think this should go in.


- Martin Tobias Holmedahl


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


On July 17, 2016, 8:14 p.m., Martin Tobias Holmedahl Sandsmark wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128473/
> ---
> 
> (Updated July 17, 2016, 8:14 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Repository: plasma-integration
> 
> 
> Description
> ---
> 
> If the status notifier item host is not available, KSNI tries to create a 
> normal QSystemTrayIcon.
> 
> The plasma platform plugin uses KSNI when it is called to create a 
> QPlatformSystemTrayIcon.
> 
> So if the status notifier item host for any reason was unavailable, this 
> would recursively run forever (assuming a turing machine with infinite 
> memory).
> 
> 
> Diffs
> -
> 
>   src/platformtheme/kdeplatformsystemtrayicon.h 6825b4d 
>   src/platformtheme/kdeplatformsystemtrayicon.cpp 0e82385 
>   src/platformtheme/kdeplatformtheme.cpp 5f0407c 
> 
> Diff: https://git.reviewboard.kde.org/r/128473/diff/
> 
> 
> Testing
> ---
> 
> Now it is possible to run applications that have tray icons with the plasma 
> platform plugin even when the status notifier item host is down or 
> unavailable.
> 
> 
> Thanks,
> 
> Martin Tobias Holmedahl Sandsmark
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 128491: Give more useful names to the acpi thermal zones and cooling devices

[Martin Tobias Holmedahl Sandsmark]

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

(Updated July 21, 2016, 2:25 p.m.)


Review request for Plasma and John Tapsell.


Repository: ksysguard


Description
---

Also fix the fact that cooling devices have a percentage of activity, they're 
not boolean on or off.

I'll reverse the tabs->spaces later, if needed.


Diffs (updated)
-

  ksysguardd/Linux/acpi.h e50b281 
  ksysguardd/Linux/acpi.c acf54e1 

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


Testing
---

It works very well.


Thanks,

Martin Tobias Holmedahl Sandsmark

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 128491: Give more useful names to the acpi thermal zones and cooling devices

[Martin Tobias Holmedahl Sandsmark]

> On July 21, 2016, 5:44 a.m., Martin Gräßlin wrote:
> > How would one test this?
> 
> Martin Gräßlin wrote:
> I mean manual testing. If I pull in the patch, what do I need to do in 
> ksysguard to actually see it in action?

make a new tab. open the sensor browser, and look under ACPI/{Cooling 
Device,Thermal Zone}, they should now have more descriptive names than just "0" 
or "1".

if you pull some of them out you should also have more descriptive names under 
the graph, instead of e. g. «-1 boolean of 1 boolean».


> On July 21, 2016, 5:44 a.m., Martin Gräßlin wrote:
> > ksysguardd/Linux/acpi.h, line 45
> > <https://git.reviewboard.kde.org/r/128491/diff/2/?file=472109#file472109line45>
> >
> > I don't see printFanState in the new code any more. Is it removed?

it should still be used by the legacy fan stuff. But I don't think I uploaded 
the latest patch.


- Martin Tobias Holmedahl


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


On July 21, 2016, 1:13 a.m., Martin Tobias Holmedahl Sandsmark wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128491/
> ---
> 
> (Updated July 21, 2016, 1:13 a.m.)
> 
> 
> Review request for Plasma and John Tapsell.
> 
> 
> Repository: ksysguard
> 
> 
> Description
> ---
> 
> Also fix the fact that cooling devices have a percentage of activity, they're 
> not boolean on or off.
> 
> I'll reverse the tabs->spaces later, if needed.
> 
> 
> Diffs
> -
> 
>   ksysguardd/Linux/acpi.h e50b281 
>   ksysguardd/Linux/acpi.c acf54e1 
> 
> Diff: https://git.reviewboard.kde.org/r/128491/diff/
> 
> 
> Testing
> ---
> 
> It works very well.
> 
> 
> Thanks,
> 
> Martin Tobias Holmedahl Sandsmark
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 128491: Give more useful names to the acpi thermal zones and cooling devices

[Martin Tobias Holmedahl Sandsmark]

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

(Updated July 21, 2016, 1:13 a.m.)


Review request for Plasma and John Tapsell.


Changes
---

Fixed the naming, unbroke the old /proc/acpi stuff.


Repository: ksysguard


Description
---

Also fix the fact that cooling devices have a percentage of activity, they're 
not boolean on or off.

I'll reverse the tabs->spaces later, if needed.


Diffs (updated)
-

  ksysguardd/Linux/acpi.h e50b281 
  ksysguardd/Linux/acpi.c acf54e1 

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


Testing
---

It works very well.


Thanks,

Martin Tobias Holmedahl Sandsmark

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 128491: Give more useful names to the acpi thermal zones and cooling devices

[Martin Tobias Holmedahl Sandsmark]

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

(Updated July 21, 2016, 12:13 a.m.)


Review request for Plasma and John Tapsell.


Repository: ksysguard


Description (updated)
---

Also fix the fact that cooling devices have a percentage of activity, they're 
not boolean on or off.

I'll reverse the tabs->spaces later, if needed.


Diffs
-

  ksysguardd/Linux/acpi.h e50b281 
  ksysguardd/Linux/acpi.c acf54e1 

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


Testing
---

It works very well.


Thanks,

Martin Tobias Holmedahl Sandsmark

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Review Request 128491: Give more useful names to the acpi thermal zones and cooling devices

[Martin Tobias Holmedahl Sandsmark]

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

Review request for Plasma and John Tapsell.


Repository: ksysguard


Description
---

Also fix the fact that cooling devices have a percentage of activity, they're 
not boolean on or off.


Diffs
-

  ksysguardd/Linux/acpi.h e50b281 
  ksysguardd/Linux/acpi.c acf54e1 

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


Testing
---

It works very well.


Thanks,

Martin Tobias Holmedahl Sandsmark

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 128473: Avoid recursive calls to QPlatformTheme::createPlatformSystemTrayIcon()

[Martin Tobias Holmedahl Sandsmark]

> On July 18, 2016, 5:49 a.m., Martin Gräßlin wrote:
> > I acknowledge the problem in general, but I think the solution is wrong as 
> > this creates now a race condition on startup where apps don't show up in 
> > the systray at all. That is if an application tries to create a systray 
> > icon before Plasma is started.
> 
> Martin Tobias Holmedahl Sandsmark wrote:
> it still creates a systray icon, it just creates an "old style" tray icon.
> 
> Martin Gräßlin wrote:
> > it still creates a systray icon, it just creates an "old style" tray 
> icon.
> 
> Which won't work on Wayland. And yes that's a valid point as we need to 
> think further than 3 months ;-)
> 
> Martin Gräßlin wrote:
> actually it won't even work on X11 as if Plasma is not up yet, neither 
> will be the xembed proxy. Which means no systray and Qt won't create it.
> 
> Martin Tobias Holmedahl Sandsmark wrote:
> won't it jump into the xembed proxy when it appears? I seem to recall 
> that happening, but I might be wrong.
> 
> Martin Tobias Holmedahl Sandsmark wrote:
> and regarding wayland, what does the "normal" platform plugin for wayland 
> do with QPlatformSystemTrayIcon?
> 
> Martin Tobias Holmedahl Sandsmark wrote:
> well, I looked in qtbase, and apparently Qt has a SNI implementation, so 
> the fallback here should work under wayland?
> 
> David Edmundson wrote:
> The Qt SNI won't work.
> 
> That's in QGenericUnixTheme - however because we load our platform theme 
> (which subclasses QPlatformTheme) we don't load it - and it's private API so 
> can't (without changing Qt)

Anyhow, you don't agree that it is better for the application to not get a tray 
icon than recursing like this and not showing up at all and spinning like crazy?


- Martin Tobias Holmedahl


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


On July 17, 2016, 8:14 p.m., Martin Tobias Holmedahl Sandsmark wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128473/
> ---
> 
> (Updated July 17, 2016, 8:14 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Repository: plasma-integration
> 
> 
> Description
> ---
> 
> If the status notifier item host is not available, KSNI tries to create a 
> normal QSystemTrayIcon.
> 
> The plasma platform plugin uses KSNI when it is called to create a 
> QPlatformSystemTrayIcon.
> 
> So if the status notifier item host for any reason was unavailable, this 
> would recursively run forever (assuming a turing machine with infinite 
> memory).
> 
> 
> Diffs
> -
> 
>   src/platformtheme/kdeplatformsystemtrayicon.h 6825b4d 
>   src/platformtheme/kdeplatformsystemtrayicon.cpp 0e82385 
>   src/platformtheme/kdeplatformtheme.cpp 5f0407c 
> 
> Diff: https://git.reviewboard.kde.org/r/128473/diff/
> 
> 
> Testing
> ---
> 
> Now it is possible to run applications that have tray icons with the plasma 
> platform plugin even when the status notifier item host is down or 
> unavailable.
> 
> 
> Thanks,
> 
> Martin Tobias Holmedahl Sandsmark
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 128473: Avoid recursive calls to QPlatformTheme::createPlatformSystemTrayIcon()

[Martin Tobias Holmedahl Sandsmark]

> On July 19, 2016, 4:02 a.m., David Edmundson wrote:
> > KStatusNotifierItemPrivate::setLegacySystemTrayEnabled(bool enabled)
> > already has a recursion check added in 
> > b45544f3d4dd9cb1873b92a609f45a68f5f6e471  (in knotifications) - which 
> > basically checks if we're using the KDE platform theme (albeit in a 
> > slightly weird way)
> > 
> > Not saying yours is "worse" but we don't want two fixes in two places.
> > 
> > Could you check you have that patch? and why it doesn't work?
> 
> Martin Tobias Holmedahl Sandsmark wrote:
> firstly, as you said, it checks in a weird way, which doesn't work, 
> that's why I thought it made more sense to fix it in the platform theme 
> itself which already knows that it is loaded and whether an SNI host is 
> available.
> 
> (fwiw, qApp->platformName() is not correct either, that's what I thought 
> was the "proper" way to do it)

patching around that also leads to no legacy tray icon being created at all, 
which is obviously wrong.


- Martin Tobias Holmedahl


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


On July 17, 2016, 8:14 p.m., Martin Tobias Holmedahl Sandsmark wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128473/
> ---
> 
> (Updated July 17, 2016, 8:14 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Repository: plasma-integration
> 
> 
> Description
> ---
> 
> If the status notifier item host is not available, KSNI tries to create a 
> normal QSystemTrayIcon.
> 
> The plasma platform plugin uses KSNI when it is called to create a 
> QPlatformSystemTrayIcon.
> 
> So if the status notifier item host for any reason was unavailable, this 
> would recursively run forever (assuming a turing machine with infinite 
> memory).
> 
> 
> Diffs
> -
> 
>   src/platformtheme/kdeplatformsystemtrayicon.h 6825b4d 
>   src/platformtheme/kdeplatformsystemtrayicon.cpp 0e82385 
>   src/platformtheme/kdeplatformtheme.cpp 5f0407c 
> 
> Diff: https://git.reviewboard.kde.org/r/128473/diff/
> 
> 
> Testing
> ---
> 
> Now it is possible to run applications that have tray icons with the plasma 
> platform plugin even when the status notifier item host is down or 
> unavailable.
> 
> 
> Thanks,
> 
> Martin Tobias Holmedahl Sandsmark
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 128473: Avoid recursive calls to QPlatformTheme::createPlatformSystemTrayIcon()

[Martin Tobias Holmedahl Sandsmark]

> On July 19, 2016, 4:02 a.m., David Edmundson wrote:
> > KStatusNotifierItemPrivate::setLegacySystemTrayEnabled(bool enabled)
> > already has a recursion check added in 
> > b45544f3d4dd9cb1873b92a609f45a68f5f6e471  (in knotifications) - which 
> > basically checks if we're using the KDE platform theme (albeit in a 
> > slightly weird way)
> > 
> > Not saying yours is "worse" but we don't want two fixes in two places.
> > 
> > Could you check you have that patch? and why it doesn't work?

firstly, as you said, it checks in a weird way, which doesn't work, that's why 
I thought it made more sense to fix it in the platform theme itself which 
already knows that it is loaded and whether an SNI host is available.

(fwiw, qApp->platformName() is not correct either, that's what I thought was 
the "proper" way to do it)


- Martin Tobias Holmedahl


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


On July 17, 2016, 8:14 p.m., Martin Tobias Holmedahl Sandsmark wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128473/
> ---
> 
> (Updated July 17, 2016, 8:14 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Repository: plasma-integration
> 
> 
> Description
> ---
> 
> If the status notifier item host is not available, KSNI tries to create a 
> normal QSystemTrayIcon.
> 
> The plasma platform plugin uses KSNI when it is called to create a 
> QPlatformSystemTrayIcon.
> 
> So if the status notifier item host for any reason was unavailable, this 
> would recursively run forever (assuming a turing machine with infinite 
> memory).
> 
> 
> Diffs
> -
> 
>   src/platformtheme/kdeplatformsystemtrayicon.h 6825b4d 
>   src/platformtheme/kdeplatformsystemtrayicon.cpp 0e82385 
>   src/platformtheme/kdeplatformtheme.cpp 5f0407c 
> 
> Diff: https://git.reviewboard.kde.org/r/128473/diff/
> 
> 
> Testing
> ---
> 
> Now it is possible to run applications that have tray icons with the plasma 
> platform plugin even when the status notifier item host is down or 
> unavailable.
> 
> 
> Thanks,
> 
> Martin Tobias Holmedahl Sandsmark
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 128473: Avoid recursive calls to QPlatformTheme::createPlatformSystemTrayIcon()

[Martin Tobias Holmedahl Sandsmark]

> On July 18, 2016, 5:49 a.m., Martin Gräßlin wrote:
> > I acknowledge the problem in general, but I think the solution is wrong as 
> > this creates now a race condition on startup where apps don't show up in 
> > the systray at all. That is if an application tries to create a systray 
> > icon before Plasma is started.
> 
> Martin Tobias Holmedahl Sandsmark wrote:
> it still creates a systray icon, it just creates an "old style" tray icon.
> 
> Martin Gräßlin wrote:
> > it still creates a systray icon, it just creates an "old style" tray 
> icon.
> 
> Which won't work on Wayland. And yes that's a valid point as we need to 
> think further than 3 months ;-)
> 
> Martin Gräßlin wrote:
> actually it won't even work on X11 as if Plasma is not up yet, neither 
> will be the xembed proxy. Which means no systray and Qt won't create it.
> 
> Martin Tobias Holmedahl Sandsmark wrote:
> won't it jump into the xembed proxy when it appears? I seem to recall 
> that happening, but I might be wrong.
> 
> Martin Tobias Holmedahl Sandsmark wrote:
> and regarding wayland, what does the "normal" platform plugin for wayland 
> do with QPlatformSystemTrayIcon?

well, I looked in qtbase, and apparently Qt has a SNI implementation, so the 
fallback here should work under wayland?


- Martin Tobias Holmedahl


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


On July 17, 2016, 8:14 p.m., Martin Tobias Holmedahl Sandsmark wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128473/
> ---
> 
> (Updated July 17, 2016, 8:14 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Repository: plasma-integration
> 
> 
> Description
> ---
> 
> If the status notifier item host is not available, KSNI tries to create a 
> normal QSystemTrayIcon.
> 
> The plasma platform plugin uses KSNI when it is called to create a 
> QPlatformSystemTrayIcon.
> 
> So if the status notifier item host for any reason was unavailable, this 
> would recursively run forever (assuming a turing machine with infinite 
> memory).
> 
> 
> Diffs
> -
> 
>   src/platformtheme/kdeplatformsystemtrayicon.h 6825b4d 
>   src/platformtheme/kdeplatformsystemtrayicon.cpp 0e82385 
>   src/platformtheme/kdeplatformtheme.cpp 5f0407c 
> 
> Diff: https://git.reviewboard.kde.org/r/128473/diff/
> 
> 
> Testing
> ---
> 
> Now it is possible to run applications that have tray icons with the plasma 
> platform plugin even when the status notifier item host is down or 
> unavailable.
> 
> 
> Thanks,
> 
> Martin Tobias Holmedahl Sandsmark
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 128473: Avoid recursive calls to QPlatformTheme::createPlatformSystemTrayIcon()

[Martin Tobias Holmedahl Sandsmark]

> On July 18, 2016, 5:49 a.m., Martin Gräßlin wrote:
> > I acknowledge the problem in general, but I think the solution is wrong as 
> > this creates now a race condition on startup where apps don't show up in 
> > the systray at all. That is if an application tries to create a systray 
> > icon before Plasma is started.
> 
> Martin Tobias Holmedahl Sandsmark wrote:
> it still creates a systray icon, it just creates an "old style" tray icon.
> 
> Martin Gräßlin wrote:
> > it still creates a systray icon, it just creates an "old style" tray 
> icon.
> 
> Which won't work on Wayland. And yes that's a valid point as we need to 
> think further than 3 months ;-)
> 
> Martin Gräßlin wrote:
> actually it won't even work on X11 as if Plasma is not up yet, neither 
> will be the xembed proxy. Which means no systray and Qt won't create it.
> 
> Martin Tobias Holmedahl Sandsmark wrote:
> won't it jump into the xembed proxy when it appears? I seem to recall 
> that happening, but I might be wrong.

and regarding wayland, what does the "normal" platform plugin for wayland do 
with QPlatformSystemTrayIcon?


- Martin Tobias Holmedahl


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


On July 17, 2016, 8:14 p.m., Martin Tobias Holmedahl Sandsmark wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128473/
> ---
> 
> (Updated July 17, 2016, 8:14 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Repository: plasma-integration
> 
> 
> Description
> ---
> 
> If the status notifier item host is not available, KSNI tries to create a 
> normal QSystemTrayIcon.
> 
> The plasma platform plugin uses KSNI when it is called to create a 
> QPlatformSystemTrayIcon.
> 
> So if the status notifier item host for any reason was unavailable, this 
> would recursively run forever (assuming a turing machine with infinite 
> memory).
> 
> 
> Diffs
> -
> 
>   src/platformtheme/kdeplatformsystemtrayicon.h 6825b4d 
>   src/platformtheme/kdeplatformsystemtrayicon.cpp 0e82385 
>   src/platformtheme/kdeplatformtheme.cpp 5f0407c 
> 
> Diff: https://git.reviewboard.kde.org/r/128473/diff/
> 
> 
> Testing
> ---
> 
> Now it is possible to run applications that have tray icons with the plasma 
> platform plugin even when the status notifier item host is down or 
> unavailable.
> 
> 
> Thanks,
> 
> Martin Tobias Holmedahl Sandsmark
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 128473: Avoid recursive calls to QPlatformTheme::createPlatformSystemTrayIcon()

[Martin Tobias Holmedahl Sandsmark]

> On July 18, 2016, 5:49 a.m., Martin Gräßlin wrote:
> > I acknowledge the problem in general, but I think the solution is wrong as 
> > this creates now a race condition on startup where apps don't show up in 
> > the systray at all. That is if an application tries to create a systray 
> > icon before Plasma is started.
> 
> Martin Tobias Holmedahl Sandsmark wrote:
> it still creates a systray icon, it just creates an "old style" tray icon.
> 
> Martin Gräßlin wrote:
> > it still creates a systray icon, it just creates an "old style" tray 
> icon.
> 
> Which won't work on Wayland. And yes that's a valid point as we need to 
> think further than 3 months ;-)
> 
> Martin Gräßlin wrote:
> actually it won't even work on X11 as if Plasma is not up yet, neither 
> will be the xembed proxy. Which means no systray and Qt won't create it.

won't it jump into the xembed proxy when it appears? I seem to recall that 
happening, but I might be wrong.


- Martin Tobias Holmedahl


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


On July 17, 2016, 8:14 p.m., Martin Tobias Holmedahl Sandsmark wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128473/
> ---
> 
> (Updated July 17, 2016, 8:14 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Repository: plasma-integration
> 
> 
> Description
> ---
> 
> If the status notifier item host is not available, KSNI tries to create a 
> normal QSystemTrayIcon.
> 
> The plasma platform plugin uses KSNI when it is called to create a 
> QPlatformSystemTrayIcon.
> 
> So if the status notifier item host for any reason was unavailable, this 
> would recursively run forever (assuming a turing machine with infinite 
> memory).
> 
> 
> Diffs
> -
> 
>   src/platformtheme/kdeplatformsystemtrayicon.h 6825b4d 
>   src/platformtheme/kdeplatformsystemtrayicon.cpp 0e82385 
>   src/platformtheme/kdeplatformtheme.cpp 5f0407c 
> 
> Diff: https://git.reviewboard.kde.org/r/128473/diff/
> 
> 
> Testing
> ---
> 
> Now it is possible to run applications that have tray icons with the plasma 
> platform plugin even when the status notifier item host is down or 
> unavailable.
> 
> 
> Thanks,
> 
> Martin Tobias Holmedahl Sandsmark
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 128473: Avoid recursive calls to QPlatformTheme::createPlatformSystemTrayIcon()

[Martin Tobias Holmedahl Sandsmark]

> On July 18, 2016, 5:49 a.m., Martin Gräßlin wrote:
> > I acknowledge the problem in general, but I think the solution is wrong as 
> > this creates now a race condition on startup where apps don't show up in 
> > the systray at all. That is if an application tries to create a systray 
> > icon before Plasma is started.

it still creates a systray icon, it just creates an "old style" tray icon.


- Martin Tobias Holmedahl


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


On July 17, 2016, 8:14 p.m., Martin Tobias Holmedahl Sandsmark wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128473/
> ---
> 
> (Updated July 17, 2016, 8:14 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Repository: plasma-integration
> 
> 
> Description
> ---
> 
> If the status notifier item host is not available, KSNI tries to create a 
> normal QSystemTrayIcon.
> 
> The plasma platform plugin uses KSNI when it is called to create a 
> QPlatformSystemTrayIcon.
> 
> So if the status notifier item host for any reason was unavailable, this 
> would recursively run forever (assuming a turing machine with infinite 
> memory).
> 
> 
> Diffs
> -
> 
>   src/platformtheme/kdeplatformsystemtrayicon.h 6825b4d 
>   src/platformtheme/kdeplatformsystemtrayicon.cpp 0e82385 
>   src/platformtheme/kdeplatformtheme.cpp 5f0407c 
> 
> Diff: https://git.reviewboard.kde.org/r/128473/diff/
> 
> 
> Testing
> ---
> 
> Now it is possible to run applications that have tray icons with the plasma 
> platform plugin even when the status notifier item host is down or 
> unavailable.
> 
> 
> Thanks,
> 
> Martin Tobias Holmedahl Sandsmark
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Review Request 128473: Avoid recursive calls to QPlatformTheme::createPlatformSystemTrayIcon()

[Martin Tobias Holmedahl Sandsmark]

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

Review request for Plasma.


Repository: plasma-integration


Description
---

If the status notifier item host is not available, KSNI tries to create a 
normal QSystemTrayIcon.

The plasma platform plugin uses KSNI when it is called to create a 
QPlatformSystemTrayIcon.

So if the status notifier item host for any reason was unavailable, this would 
recursively run forever (assuming a turing machine with infinite memory).


Diffs
-

  src/platformtheme/kdeplatformsystemtrayicon.h 6825b4d 
  src/platformtheme/kdeplatformsystemtrayicon.cpp 0e82385 
  src/platformtheme/kdeplatformtheme.cpp 5f0407c 

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


Testing
---

Now it is possible to run applications that have tray icons with the plasma 
platform plugin even when the status notifier item host is down or unavailable.


Thanks,

Martin Tobias Holmedahl Sandsmark

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 128431: Fix race condition when new applications open

[Martin Tobias Holmedahl Sandsmark]

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

(Updated July 16, 2016, 3:12 p.m.)


Status
--

This change has been marked as submitted.


Review request for Plasma and John Tapsell.


Changes
---

Submitted with commit c2fba0b64a0a2541a40a46a9f17fdbda1ea6a498 by Martin T. H. 
Sandsmark to branch master.


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


Repository: libksysguard


Description
---

When opening and closing an application quickly it sometimes got "stuck" in 
ksysguard because it was added by ProcessModel while 
Processes::processesUpdated() was running, so Processes never processed it.


Diffs
-

  processui/ProcessModel.cpp c55cea6 

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


Testing
---

It doesn't happen anymore.


Thanks,

Martin Tobias Holmedahl Sandsmark

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 128431: Fix race condition when new applications open

[Martin Tobias Holmedahl Sandsmark]

> On July 13, 2016, 10:44 a.m., Aleix Pol Gonzalez wrote:
> > Would it be possible to have a test there? It will be easier to review for 
> > us.

what would the test test?

nothing should call updateOrAddProcess(), ideally I'd just make it private, but 
I'm not sure how stable the libksysguard API is supposed to be.


- Martin Tobias Holmedahl


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


On July 12, 2016, 9:22 p.m., Martin Tobias Holmedahl Sandsmark wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128431/
> ---
> 
> (Updated July 12, 2016, 9:22 p.m.)
> 
> 
> Review request for Plasma and John Tapsell.
> 
> 
> Bugs: 261255
> https://bugs.kde.org/show_bug.cgi?id=261255
> 
> 
> Repository: libksysguard
> 
> 
> Description
> ---
> 
> When opening and closing an application quickly it sometimes got "stuck" in 
> ksysguard because it was added by ProcessModel while 
> Processes::processesUpdated() was running, so Processes never processed it.
> 
> 
> Diffs
> -
> 
>   processui/ProcessModel.cpp c55cea6 
> 
> Diff: https://git.reviewboard.kde.org/r/128431/diff/
> 
> 
> Testing
> ---
> 
> It doesn't happen anymore.
> 
> 
> Thanks,
> 
> Martin Tobias Holmedahl Sandsmark
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 128429: Fix strings in tooltips

[Martin Tobias Holmedahl Sandsmark]

> On July 12, 2016, 9:11 p.m., Albert Astals Cid wrote:
> > What difference does this make? I guess it's "just" a "nicer" view? Could 
> > you post screenshots of before and after?

(I tried posting a reply already but reviewboard seems a bit wonky, so sorry if 
you get a duplicate).

The important thing for me was that it doesn't spam «"Tag 'b' is not defined in 
message {<__kuit_internal_top__>chromiumProcess ID: 4518Parent: chromi...}."» anymore.


- Martin Tobias Holmedahl


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


On July 12, 2016, 7:24 p.m., Martin Tobias Holmedahl Sandsmark wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128429/
> ---
> 
> (Updated July 12, 2016, 7:24 p.m.)
> 
> 
> Review request for Plasma, Albert Astals Cid and John Tapsell.
> 
> 
> Repository: libksysguard
> 
> 
> Description
> ---
> 
> Changed the strings to use proper kuit markup.
> 
> 
> Diffs
> -
> 
>   processui/ProcessModel.cpp c55cea6 
> 
> Diff: https://git.reviewboard.kde.org/r/128429/diff/
> 
> 
> Testing
> ---
> 
> Viewed all the tooltips.
> 
> 
> Thanks,
> 
> Martin Tobias Holmedahl Sandsmark
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Review Request 128431: Fix race condition when new applications open

[Martin Tobias Holmedahl Sandsmark]

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

Review request for Plasma and John Tapsell.


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


Repository: libksysguard


Description
---

When opening and closing an application quickly it sometimes got "stuck" in 
ksysguard because it was added by ProcessModel while 
Processes::processesUpdated() was running, so Processes never processed it.


Diffs
-

  processui/ProcessModel.cpp c55cea6 

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


Testing
---

It doesn't happen anymore.


Thanks,

Martin Tobias Holmedahl Sandsmark

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Review Request 128429: Fix strings in tooltips

[Martin Tobias Holmedahl Sandsmark]

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

Review request for Plasma, Albert Astals Cid and John Tapsell.


Repository: libksysguard


Description
---

Changed the strings to use proper kuit markup.


Diffs
-

  processui/ProcessModel.cpp c55cea6 

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


Testing
---

Viewed all the tooltips.


Thanks,

Martin Tobias Holmedahl Sandsmark

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 128164: Use QIcon for background icon for folder thumbnails

[Martin Tobias Holmedahl Sandsmark]

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

(Updated June 13, 2016, 8:56 a.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks and Plasma.


Changes
---

Submitted with commit cfcb9f757111a55f6aec23ef97a4b5c03e234576 by Martin T. H. 
Sandsmark to branch master.


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


Repository: kio-extras


Description
---

For consistency when using a QPA that doesn't use KIconLoader, use QIcon 
instead of KIconLoader for getting the background icon for folder thumbnails.

This is visible e. g. in Dolphin when running under a "foreign" session without 
the plasma-integration QPA: https://bugsfiles.kde.org/attachment.cgi?id=99476

Adding the plasma group to the reviewers, as that was who bugzilla sent to.


Diffs
-

  thumbnail/thumbnail.cpp f63fe25 

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


Testing
---

Tried running both without: 
https://iskrembilen.com/screenshots/withoutplasmaintegration.png

and with plasma-integration: 
https://iskrembilen.com/screenshots/withplasmaintegration.png


Thanks,

Martin Tobias Holmedahl Sandsmark

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Review Request 128164: Use QIcon for background icon for folder thumbnails

[Martin Tobias Holmedahl Sandsmark]

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

Review request for KDE Frameworks and Plasma.


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


Repository: kio-extras


Description
---

For consistency when using a QPA that doesn't use KIconLoader, use QIcon 
instead of KIconLoader for getting the background icon for folder thumbnails.

This is visible e. g. in Dolphin when running under a "foreign" session without 
the plasma-integration QPA: https://bugsfiles.kde.org/attachment.cgi?id=99476

Adding the plasma group to the reviewers, as that was who bugzilla sent to.


Diffs
-

  thumbnail/thumbnail.cpp f63fe25 

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


Testing
---

Tried running both without: 
https://iskrembilen.com/screenshots/withoutplasmaintegration.png

and with plasma-integration: 
https://iskrembilen.com/screenshots/withplasmaintegration.png


Thanks,

Martin Tobias Holmedahl Sandsmark

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 125662: mangonel: Initial port to Qt5/KF5

[Martin Tobias Holmedahl Sandsmark]

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

Ship it!


Ship It!

- Martin Tobias Holmedahl Sandsmark


On Oct. 16, 2015, 4:04 p.m., Jeremy Whiting wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125662/
> ---
> 
> (Updated Oct. 16, 2015, 4:04 p.m.)
> 
> 
> Review request for Plasma and Martin Tobias Holmedahl Sandsmark.
> 
> 
> Repository: mangonel
> 
> 
> Description
> ---
> 
> Port of Mangonel to Qt5/KF5
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt fec95da 
>   Label.cpp 136f70b 
>   Mangonel.h 9c8a32f 
>   Mangonel.cpp 63e10ab 
>   main.cpp 1b0c4a3 
>   providers/Applications.cpp 7674a97 
>   providers/Paths.cpp 713aba3 
> 
> Diff: https://git.reviewboard.kde.org/r/125662/diff/
> 
> 
> Testing
> ---
> 
> It builds and runs, but looks a bit strange since it's a plani QWidget. 
> (Maybe we could say it's using Material design and call it good, dunno :p)
> 
> 
> File Attachments
> 
> 
> screenshot
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2015/10/16/2f8f36d8-dea2-4679-8826-96ebbaa8c8e9__mangonel.png
> 
> 
> Thanks,
> 
> Jeremy Whiting
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 111849: custom first week for plasmaclock.

[Martin Tobias Holmedahl Sandsmark]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/111849/#review38188
---


isn't this duplicating the functionality in system settings → locale → 
country/region  language → calendar → first day of the week?

- Martin Tobias Holmedahl Sandsmark


On Aug. 20, 2013, 4:37 a.m., Hu Zheng wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/111849/
 ---
 
 (Updated Aug. 20, 2013, 4:37 a.m.)
 
 
 Review request for Plasma.
 
 
 Description
 ---
 
 In normal case, the first week is the first week in the year, but the 
 teachers may want to custom the first week, for example, make the 8th week as 
 the first week, then the 9th week will be the second week, and so on. Then 
 the teacher can easily do their work as a custom teaching period!
 
 
 Diffs
 -
 
 
 Diff: http://git.reviewboard.kde.org/r/111849/diff/
 
 
 Testing
 ---
 
 cd kde-workspace-4.10.5/libs/plasmaclock
 patch -p1  customfirstweek.patch
 
 The save configuration codes are not very beautiful, but it works fine!
 
 
 File Attachments
 
 
 patch
   
 http://git.reviewboard.kde.org/media/uploaded/files/2013/08/03/customfirstweek.patch
 another patch, use custom config file.
   
 http://git.reviewboard.kde.org/media/uploaded/files/2013/08/20/customweek.patch
 
 
 Thanks,
 
 Hu Zheng
 


___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel