D24070: [WIP] Don't use toolTipMainText to show info, rather use the second line

[Matej Mrenica]

mthw accepted this revision.
mthw added a comment.


  Anyway, anytime you feel like this is finished you can land it. You don't 
have to wait for me.

REPOSITORY
  R120 Plasma Workspace

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

To: ngraham, #vdg, #plasma, ndavis, mthw
Cc: GB_2, ndavis, broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
zachus, fbampaloukas, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D4574: [WIP]: unify file drop menu and containment

[Tranter Madi]

trmdi added a comment.


  This patch seems to cause this bug: 
https://bugs.kde.org/show_bug.cgi?id=415917

REPOSITORY
  R119 Plasma Desktop

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

To: mart, #plasma, hein
Cc: trmdi, hein, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26392: Add option to easily configure and start a hotspot

[Jan Grulich]

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

INLINE COMMENTS

> apol wrote in Toolbar.qml:133
> is it always prossible to create the hotspot? I'd expect it not to be 
> possible at all sometimes (visible should be false then) and if the wifi is 
> in use it might not be able to do it? (enabled false then).

See Component.onCompleted: { }. We make this visible based on 
handler.hotspotSupported, which checks NM version and whether there is 
available wifi card. It's done this way because it's a non-notifyable property.

REPOSITORY
  R116 Plasma Network Management Applet

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

To: jgrulich, #plasma, ngraham, #vdg
Cc: alexde, mthw, apol, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, ahiemstra, mart

D26392: Add option to easily configure and start a hotspot

[Jan Grulich]

jgrulich added a comment.


  In D26392#587200 , @alexde wrote:
  
  > > UI-wise, this would probably be better off as a button than a checkbox.
  >
  > Personally, I'm more inclined to a checkbox. Why is "turn wifi on/off" not 
an action? Right now, I don't see the big difference.
  >
  > Is the new "hotspot" equivalent to the "shared wifi", you can already set 
up? 
  >  If so, without really reading the patch details yet, just some remarks or 
ideas:
  >
  > 1. If you have configured several hotspots in the KCM, you probably need to 
choose a default one or a specific one when activating the hotspot.
  
  
  This is meant to be as simple as possible, allowing users to create a hotspot 
without needing them to know the details. On Android you also don't get to 
choose.
  
  > 2. What do you think about adding a small configure button for the default 
hotspot directly on the plasma-nm frontend  next to the hotspot checkbox/button?
  >   - That would make it much easier for newcomers to use and configure their 
hotspot without first digging in the KCM.
  > 3. I also think that a hotspot (shared wifi) connection should be better 
separated from the other wifi connections in the KCM. If I create a new hotspot 
in the KCM now, it hides in the long list of known connections.
  
  I will probably use a new icon for this. I will need to request it.
  
  > 4. That's somehow also true for the plasma-nm list of available networks: 
It's not really clear that a hotspot in the list is a indeed hotspot but not 
another access point.
  
  Same here. A different icon should be enough.
  
  > In D26392#587159 , @mthw wrote:
  > 
  >> @apol You are right, it is not possible to create a hotspot if one is 
already connected to a WiFi network. Currently enabling hotspot disables your 
previous connection (WiFi) and hides available WiFi networks.
  > 
  > 
  > Is this also true if one has two wifi network cards? IIRC I could run both 
with an additional usb netwok card simultaneously.
  
  In case of multiple wifi cards we will choose the one which is not in use. 
Still any wifi card makes this option available, even if it's in use.
  
  >  ---
  > 
  > @jgrulich do you intend to create another patch to display all connected 
clients, similiar to the list of active connections? :)
  
  I don't think NetworkManager provides this information.

REPOSITORY
  R116 Plasma Network Management Applet

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

To: jgrulich, #plasma, ngraham, #vdg
Cc: alexde, mthw, apol, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, ahiemstra, mart

D26392: Add option to easily configure and start a hotspot

[Jan Grulich]

jgrulich added a comment.


  In D26392#587159 , @mthw wrote:
  
  > @apol You are right, it is not possible to create a hotspot if one is 
already connected to a WiFi network. Currently enabling hotspot disables your 
previous connection (WiFi) and hides available WiFi networks.
  
  
  That's a question. Do we want to allow to create a hotspot if users are 
already connected to w WiFi network?  You are allowed to do this for example on 
Android. Users might not realize why they are not allowed to create a hotspot 
and disconnecting from WiFi doesn't seem to me be a good first step.

REPOSITORY
  R116 Plasma Network Management Applet

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

To: jgrulich, #plasma, ngraham, #vdg
Cc: alexde, mthw, apol, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, ahiemstra, mart

D26447: Dynamically show and hide based on whether or not any vaults are configured

[Nathaniel Graham]

ngraham updated this revision to Diff 72841.
ngraham marked 3 inline comments as done.
ngraham added a comment.


  Address more review comments (eventually I'll get this right...)

REPOSITORY
  R845 Plasma Vault

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26447?vs=72840=72841

BRANCH
  dynamically-show-and-hide (branched from master)

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

AFFECTED FILES
  plasma/package/contents/ui/main.qml
  plasma/vaultsmodel.cpp
  plasma/vaultsmodel.h

To: ngraham, broulik, ivan, #plasma, #vdg
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26447: Dynamically show and hide based on whether or not any vaults are configured

[Kai Uwe Broulik]

broulik added a comment.


  +1

INLINE COMMENTS

> vaultsmodel.h:36
>  Q_PROPERTY(bool hasError READ hasError NOTIFY hasErrorChanged)
> +Q_PROPERTY(int rowCount READ rowCount NOTIFY rowCountChanged)
>  

The property name should stay `count` to avoid conflict with `rowCount` which 
us invokable

> vaultsmodel.h:45
>  
> -int rowCount(const QModelIndex ) const override;
> +int rowCount(const QModelIndex =QModelIndex()) const override;
>  

Coding style

> vaultsmodel.h:96
>  void hasErrorChanged(bool hasError);
> +void rowCountChanged(int count);
>  

Imho this argument isn't necessary but it's probably consistent with the rest

REPOSITORY
  R845 Plasma Vault

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

To: ngraham, broulik, ivan, #plasma, #vdg
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26447: Dynamically show and hide based on whether or not any vaults are configured

[Nathaniel Graham]

ngraham updated this revision to Diff 72840.
ngraham marked 2 inline comments as done.
ngraham added a comment.


  Address review comments

REPOSITORY
  R845 Plasma Vault

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26447?vs=72839=72840

BRANCH
  dynamically-show-and-hide (branched from master)

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

AFFECTED FILES
  plasma/package/contents/ui/main.qml
  plasma/vaultsmodel.cpp
  plasma/vaultsmodel.h

To: ngraham, broulik, ivan, #plasma, #vdg
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26447: Dynamically show and hide based on whether or not any vaults are configured

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> vaultsmodel.cpp:103
>  
> +if (vaultKeys.size() > 0) {
> +emit q->countChanged(vaultKeys.size());

You probably want to remember the old count and emit when it's different. The 
model could have been full and become empty after all.

> vaultsmodel.h:45
>  
>  int rowCount(const QModelIndex ) const override;
>  

If you give `parent` a default argument you can just use that in your property 
rather than adding a dedicated `count` method

REPOSITORY
  R845 Plasma Vault

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

To: ngraham, broulik, ivan, #plasma, #vdg
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26447: Dynamically show and hide based on whether or not any vaults are configured

[Nathaniel Graham]

ngraham created this revision.
ngraham added reviewers: broulik, ivan, Plasma, VDG.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
ngraham requested review of this revision.

REVISION SUMMARY
  Currently, the Vaults icon is always visible in the System Tray. However most 
tray icons
  dynamically show and hide themselves based on whether or not they're relevant 
and in use,
  which results in a nice clean clutter-free System Tray. So it would be nice 
for Vaults
  to do the same.
  
  This patch implements that behavior by adding a `count` property that gets 
updated based
  on vault creation and deletion, and making the icon only appear in the tray 
when the count
  is greater than 0. People who prefer to have it always visible can of course 
override
  this in the System Tray settings, but the auto-hide behavior in this patch is 
probably a
  better default for most.

TEST PLAN
  1. Have no vaults -> icon is only in the popup
  2. Create a vault -> icon appears in the tray
  3. Delete the vault -> icon disappears from the tray and lives in the popup 
again

REPOSITORY
  R845 Plasma Vault

BRANCH
  dynamically-show-and-hide (branched from master)

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

AFFECTED FILES
  plasma/package/contents/ui/main.qml
  plasma/vaultsmodel.cpp
  plasma/vaultsmodel.h

To: ngraham, broulik, ivan, #plasma, #vdg
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26438: Runners: Convert foreach to for

[David Edmundson]

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


  Cool, thanks

INLINE COMMENTS

> fetchsqlite.cpp:100
>  query.prepare(sql);
> -foreach(const QString , bindObjects.keys()) {
> +for(const QString  : bindObjects.keys()) {
>  query.bindValue(variableName, bindObjects.value(variableName));

There's an optimisation available here

> windowsrunner.cpp:78
>  
> -foreach (const WId w, KWindowSystem::windows()) {
> +for (const WId w : KWindowSystem::windows()) {
>  KWindowInfo info(w, NET::WMWindowType | NET::WMDesktop |

Does this detach?

REPOSITORY
  R120 Plasma Workspace

BRANCH
  master

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

To: meven, #plasma, davidedmundson
Cc: davidedmundson, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26445: [KRunner KCM] Mark KCM as dirty when plugin configuration changes

[Kai Uwe Broulik]

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

REVISION SUMMARY
  Connect `configCommitted` to ensure "Apply" gets enabled when you change any 
of the runner plugin settings.
  Also, clean up the other connect.

TEST PLAN
  - Connecting to `dirty` insted of `markAsDirty` so it can go into 5.17
  
  - Opened KCM, opened dictionary runner settings, changed keyword, "Apply" not 
got enabled as expected

REPOSITORY
  R119 Plasma Desktop

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

AFFECTED FILES
  kcms/runners/kcm.cpp

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

D26444: [RunnerResultsModel] Watch krunnerrc and reparse configuration when it changed

[Kai Uwe Broulik]

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

REVISION SUMMARY
  This was present in the old `SourcesModel` but not ported to the new one.

TEST PLAN
  5.17
  Noticed that changing runner settings didn't update krunner live.
  The KCM still doesn't enable Apply properly but that is separate. When I hit 
OK things work immediately as expected now.

REPOSITORY
  R112 Milou

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

AFFECTED FILES
  lib/runnerresultsmodel.cpp
  lib/runnerresultsmodel.h

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

D24070: [WIP] Don't use toolTipMainText to show info, rather use the second line

[Nathaniel Graham]

ngraham added a comment.


  Oops, good catch.

REPOSITORY
  R120 Plasma Workspace

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

To: ngraham, #vdg, #plasma, ndavis, mthw
Cc: GB_2, ndavis, broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
zachus, fbampaloukas, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D24070: [WIP] Don't use toolTipMainText to show info, rather use the second line

[Nathaniel Graham]

ngraham updated this revision to Diff 72816.
ngraham added a comment.


  Also show "Power management is disabled" text when there are no batteries

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24070?vs=72812=72816

BRANCH
  arcpatch-D24070

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

AFFECTED FILES
  applets/batterymonitor/package/contents/ui/batterymonitor.qml

To: ngraham, #vdg, #plasma, ndavis, mthw
Cc: GB_2, ndavis, broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
zachus, fbampaloukas, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D24070: [WIP] Don't use toolTipMainText to show info, rather use the second line

[Matej Mrenica]

mthw added a comment.


  Power management disabled message is not show anymore if there is no battery 
present. Is that intentional?

REPOSITORY
  R120 Plasma Workspace

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

To: ngraham, #vdg, #plasma, ndavis, mthw
Cc: GB_2, ndavis, broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
zachus, fbampaloukas, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26386: Kicker/RecentDocuments: add icons to actions

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a comment.


  Nice!

REPOSITORY
  R120 Plasma Workspace

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

To: meven, #plasma, ngraham, hein
Cc: hein, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D24070: [WIP] Don't use toolTipMainText to show info, rather use the second line

[Nathaniel Graham]

ngraham updated this revision to Diff 72812.
ngraham edited the test plan for this revision.
ngraham added a comment.


  --whitespace changes

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24070?vs=72811=72812

BRANCH
  arcpatch-D24070

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

AFFECTED FILES
  applets/batterymonitor/package/contents/ui/batterymonitor.qml

To: ngraham, #vdg, #plasma, ndavis, mthw
Cc: GB_2, ndavis, broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
zachus, fbampaloukas, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D24070: Don't use toolTipMainText to show info, rather use the second line

[Nathaniel Graham]

ngraham updated this revision to Diff 72811.
ngraham added a comment.


  Update according to review comments and tweak presentation

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24070?vs=71222=72811

BRANCH
  arcpatch-D24070

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

AFFECTED FILES
  applets/batterymonitor/package/contents/ui/batterymonitor.qml

To: ngraham, #vdg, #plasma, ndavis, mthw
Cc: GB_2, ndavis, broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
zachus, fbampaloukas, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26438: Runners: Convert foreach to for

[Méven Car]

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

REPOSITORY
  R120 Plasma Workspace

BRANCH
  master

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

AFFECTED FILES
  runners/activities/activityrunner.cpp
  runners/appstream/appstreamrunner.cpp
  runners/bookmarks/bookmarksrunner.cpp
  runners/bookmarks/browsers/chrome.cpp
  runners/bookmarks/browsers/chromefindprofile.cpp
  runners/bookmarks/browsers/firefox.cpp
  runners/bookmarks/browsers/opera.cpp
  runners/bookmarks/fetchsqlite.cpp
  runners/kill/killrunner.cpp
  runners/powerdevil/PowerDevilRunner.cpp
  runners/recentdocuments/recentdocuments.cpp
  runners/services/servicerunner.cpp
  runners/sessions/sessionrunner.cpp
  runners/windowedwidgets/windowedwidgetsrunner.cpp
  runners/windows/windowsrunner.cpp

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

D26434: [Battery Monitor] Don't blink when battery is critical

[Kai Uwe Broulik]

This revision was automatically updated to reflect the committed changes.
Closed by commit R120:222b407454f7: [Battery Monitor] Dont blink when 
battery is critical (authored by broulik).

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26434?vs=72794=72806

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

AFFECTED FILES
  applets/batterymonitor/package/contents/ui/batterymonitor.qml

To: broulik, #plasma, anthonyfieroni, ngraham
Cc: ngraham, anthonyfieroni, plasma-devel, LeGast00n, The-Feren-OS-Dev, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26386: Kicker/RecentDocuments: add icons to actions

[Méven Car]

meven updated this revision to Diff 72804.
meven marked 2 inline comments as done.
meven added a comment.


  Add icon identity for showContactInfo action

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26386?vs=72755=72804

BRANCH
  arcpatch-D26386_1

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

AFFECTED FILES
  applets/kicker/plugin/actionlist.cpp
  applets/kicker/plugin/actionlist.h
  applets/kicker/plugin/appentry.cpp
  applets/kicker/plugin/appsmodel.cpp
  applets/kicker/plugin/contactentry.cpp
  applets/kicker/plugin/recentcontactsmodel.cpp
  applets/kicker/plugin/recentusagemodel.cpp
  applets/kicker/plugin/rootmodel.cpp
  applets/kicker/plugin/runnermatchesmodel.cpp

To: meven, #plasma, ngraham, hein
Cc: hein, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26386: Kicker/RecentDocuments: add icons to actions

[Nathaniel Graham]

ngraham added inline comments.

INLINE COMMENTS

> meven wrote in contactentry.cpp:117
> I missing an icon for this action, I am open for suggestion.

Maybe `user-identity` or `identity`, depending on what it does

REPOSITORY
  R120 Plasma Workspace

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

To: meven, #plasma, ngraham, hein
Cc: hein, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26434: [Battery Monitor] Don't blink when battery is critical

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a comment.


  > Especially when you're almost out of juice you don't want to be wasting it 
blinking an icon causing excess repaints.
  
  lol

REPOSITORY
  R120 Plasma Workspace

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

To: broulik, #plasma, anthonyfieroni, ngraham
Cc: ngraham, anthonyfieroni, plasma-devel, LeGast00n, The-Feren-OS-Dev, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26379: [KCMs/Notifications] Tweak layout and strings to have a bit more visual grouping for the logical sections

[Nathaniel Graham]

This revision was automatically updated to reflect the committed changes.
Closed by commit R119:f70092e5bc75: [KCMs/Notifications] Tweak layout and 
strings to have a bit more visual… (authored by ngraham).

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26379?vs=72690=72801

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

AFFECTED FILES
  kcms/notifications/package/contents/ui/main.qml

To: ngraham, broulik, #plasma, #vdg, ndavis
Cc: ndavis, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D24070: Don't use toolTipMainText to show info, rather use the second line

[Nathaniel Graham]

ngraham planned changes to this revision.
ngraham added a comment.


  I'm not done yet. ;-)

REPOSITORY
  R120 Plasma Workspace

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

To: ngraham, #vdg, #plasma, ndavis, mthw
Cc: GB_2, ndavis, broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
zachus, fbampaloukas, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26434: [Battery Monitor] Don't blink when battery is critical

[Kai Uwe Broulik]

broulik added a comment.


  Works here. `notify-send foo bar -u critical` will show on top of everything.

REPOSITORY
  R120 Plasma Workspace

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

To: broulik, #plasma, anthonyfieroni
Cc: anthonyfieroni, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26434: [Battery Monitor] Don't blink when battery is critical

[Anthony Fieroni]

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


  > Given battery critical notification is persistent and always on top of 
everything these days, you can't really miss the fact that you're almost ouf of 
power.
  
  I investigate on it, but if you watch full screen video (Falkon) it does not 
appear on top, for some reason.

REPOSITORY
  R120 Plasma Workspace

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

To: broulik, #plasma, anthonyfieroni
Cc: anthonyfieroni, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26275: SourcesPage: Override contentItem of ListSectionHeader instead of relying on data property

[Arjen Hiemstra]

ahiemstra abandoned this revision.
ahiemstra added a comment.


  > The bug mentioned got fixed by this (old Qt doesn't iterate over QList).
  > 
  > I don't see how this would make it look any different. The kirigami visible 
patch is indeed necessary but for an entirely different reason.
  
  You're right, it does seem to work correctly now. Weird, when I made this I 
was sure the problem was with the sizing of the ActionToolBar. But maybe I was 
put off by the overflow button visibility.

REPOSITORY
  R134 Discover Software Store

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

To: ahiemstra, #plasma, #discover_software_store, apol, ngraham
Cc: rikmills, ngraham, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26434: [Battery Monitor] Don't blink when battery is critical

[Kai Uwe Broulik]

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

REVISION SUMMARY
  Especially when you're almost out of juice you don't want to be wasting it 
blinking an icon causing excess repaints.
  Also considering the critical percentage is hardcoded to 5% here and doesn't 
follow PowerDevil's setting.
  Given battery critical notification is persistent and always on top of 
everything these days, you can't really miss the fact that you're almost ouf of 
power.

TEST PLAN
  - Fully charged: was hidden still
  - Not fully charged: was active now, never blinked, even when low

REPOSITORY
  R120 Plasma Workspace

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

AFFECTED FILES
  applets/batterymonitor/package/contents/ui/batterymonitor.qml

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

D26386: Kicker/RecentDocuments: add icons to actions

[Méven Car]

meven added inline comments.

INLINE COMMENTS

> contactentry.cpp:117
>  
> -actionList << Kicker::createActionItem(i18n("Show Contact 
> Information..."), QStringLiteral("showContactInfo"));
> +actionList << Kicker::createActionItem(i18n("Show Contact 
> Information..."), QString(), QStringLiteral("showContactInfo"));
>  

I missing an icon for this action, I am open for suggestion.

REPOSITORY
  R120 Plasma Workspace

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

To: meven, #plasma, ngraham, hein
Cc: hein, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26086: Don't fetch pixmap if we have it already

[Kai Uwe Broulik]

This revision was automatically updated to reflect the committed changes.
Closed by commit R120:08f02266c922: Dont fetch pixmap if we have it 
already (authored by broulik).

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26086?vs=71794=72785

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

AFFECTED FILES
  libtaskmanager/xwindowtasksmodel.cpp

To: broulik, #plasma, hein
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart