D20863: Allow folder view elements to be be dropped using other Action than Copy

[Nathaniel Graham]

ngraham added a comment.


  Me too, it's a bit unnerving the first few times you do it!

REPOSITORY
  R119 Plasma Desktop

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

To: meven, ngraham, hein
Cc: hein, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D20863: Allow folder view elements to be be dropped using other Action than Copy

[Méven Car]

meven added a comment.


  In D20863#461260 , @ngraham wrote:
  
  > Now don't forget to merge to master. :)
  >
  >   git checkout Plasma/5.15
  >   git pull
  >   git checkout master
  >   git merge -s recursive -Xours origin/Plasma/5.15
  >   git push
  >
  >
  >
  >
  >  ---
  >
  > I have a handy shell function `kmerge` that does this for me and then 
checks the result:
  >
  >   function kmerge {
  >   # Make sure the merge branch is up to date
  >   CURRENTBRANCH=`git branch 2> /dev/null | sed -e '/^[^*]/d' -e 's/* 
\(.*\)/\1/'`
  >   git checkout $1
  >   git pull
  >  
  >   # Do the merge safely
  >   git checkout $CURRENTBRANCH
  >   git merge -s recursive -Xours $1
  >  
  >   # Verify it afterwards
  >   git diff origin/HEAD HEAD
  >  
  >   # Now push!
  >   }
  >  
  >
  
  
  Done thanks
  
  I am always worried to merge others people work.

REPOSITORY
  R119 Plasma Desktop

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

To: meven, ngraham, hein
Cc: hein, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D21033: Panels: reset shadow pixmaps on theme change

[Friedrich W. H. Kossebau]

This revision was automatically updated to reflect the committed changes.
Closed by commit R120:93bdcda49702: Panels: reset shadow pixmaps on theme 
change (authored by kossebau).

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21033?vs=57592=57623

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

AFFECTED FILES
  shell/panelshadows.cpp

To: kossebau, #plasma, apol
Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D21012: [PanelView] Update mask as last on resize event (& move one for consistency)

[Friedrich W. H. Kossebau]

This revision was automatically updated to reflect the committed changes.
Closed by commit R120:b51fd4bab2a5: [PanelView] Update mask as last on resize 
event ( move one for consistency) (authored by kossebau).

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21012?vs=57540=57624

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

AFFECTED FILES
  shell/panelview.cpp

To: kossebau, #plasma, broulik
Cc: apol, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, mart

D21029: UpdatesPage: Don't try to translate version numbers

[Aleix Pol Gonzalez]

apol added a comment.


  Yep, change isn't correct. A solution would be to ad an installedVersion && 
availableVersion ? i18n(...) : "".
  
  Or fix i18n() to not complain about "", although I'm pretty sure it's not 
easily doable.

REPOSITORY
  R134 Discover Software Store

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

To: jbbgameich, #discover_software_store
Cc: apol, pino, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart

D21030: [WidgetExplorer] Fix blurry previews

[Nathaniel Graham]

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


  Nice! Stable branch please.

REPOSITORY
  R119 Plasma Desktop

BRANCH
  no-blurry-previews-widget-explorer (branched from master)

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

To: filipf, #plasma, #vdg, ngraham
Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D21012: [PanelView] Update mask as last on resize event (& move one for consistency)

[Kai Uwe Broulik]

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


  Makes sense and works fine, thanks

REPOSITORY
  R120 Plasma Workspace

BRANCH
  updatemasklast

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

To: kossebau, #plasma, broulik
Cc: apol, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, mart

D20830: Add hack to unbreak audio playback through pure JS via new Audio()

[Kai Uwe Broulik]

broulik added a comment.


  Looks like a recent policy change in Chrome, only my Chrome 74 seems affected 
and I recall it working half a year ago when I implemented this feature.

REPOSITORY
  R856 Plasma Browser Integration

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

To: broulik, #plasma, fvogt
Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D21030: [WidgetExplorer] Fix blurry previews

[Filip Fila]

filipf updated this revision to Diff 57617.
filipf added a comment.


  use Math.floor instead of Math.round

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21030?vs=57588=57617

BRANCH
  no-blurry-previews-widget-explorer (branched from master)

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

AFFECTED FILES
  desktoppackage/contents/explorer/WidgetExplorer.qml

To: filipf, #plasma, #vdg, ngraham
Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D21030: [WidgetExplorer] Fix blurry previews

[Nathaniel Graham]

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

INLINE COMMENTS

> WidgetExplorer.qml:295
>  keyNavigationWraps: true
> -cellWidth: (width - units.smallSpacing) / 2
> +cellWidth: Math.round((width - units.smallSpacing) / 2)
>  cellHeight: cellWidth + units.gridUnit * 4 + units.smallSpacing 
> * 2

In general it's best to use `floor` rather than `round` for this kind of thing 
to prevent everything from getting rounded up and potentially overflowing the 
bounds.

REPOSITORY
  R119 Plasma Desktop

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

To: filipf, #plasma, #vdg, ngraham
Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D21038: Also store the player's frame ID

[Kai Uwe Broulik]

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

REVISION SUMMARY
  The extension is also injected into iframes on a website. They share the same 
tab id as the rest of the website, so when having multiple videos embedded as 
iframes, they all get mpris signals relayed to them, causing e.g. a play 
command to suddenly start playing all of them.
  This patch introduces a `playerId` which is basically tabId-frameId (with "0" 
being the main page) and uses that for identifying players.
  The code is also simplified a bit by having a `sendPlayerTabMessage` which 
checks for a player being present (instead of having the caller do that 
everywhere) and then sends it to the appropriate tabId and frameId.

TEST PLAN
  I started the video at the bottom of [1], hit pause in media controller, hit 
play in media controller.
  Previously all the videos would start playing as they're separate embedded 
iframes and all got signalled the play request.
  With this patch only the relevant video is controlled. Tested play, pause, 
seeking, volume change. Closing the tab unregisters the player as normal.
  [1] https://www.winhistory.de/more/nt351.htm

REPOSITORY
  R856 Plasma Browser Integration

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

AFFECTED FILES
  extension/extension.js

To: broulik, #plasma, davidedmundson, fvogt
Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D20969: [potd] Modernize configuration settings

[Filip Fila]

filipf added a comment.


  In D20969#461288 , @ngraham wrote:
  
  > Few changes needed to the Description before landing:
  >
  > - Is the `NOTE` still accurate? I don't see that behavior anymore.
  > - Also we're no longer porting to QQC2
  
  
  Fixed. Regarding the note, nope, that was a QQC2 combobox bug.

REPOSITORY
  R114 Plasma Addons

BRANCH
  modernize-potd-config (branched from master)

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

To: filipf, #plasma, #vdg, ngraham
Cc: abetts, davidedmundson, plasma-devel, jraleigh, GB_2, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D20969: [potd] Modernize configuration settings

[Nathaniel Graham]

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


  Few changes needed to the Description before landing:
  
  - Is the `NOTE` still accurate? I don't see that behavior anymore.
  - Also we're no longer porting to QQC2

REPOSITORY
  R114 Plasma Addons

BRANCH
  modernize-potd-config (branched from master)

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

To: filipf, #plasma, #vdg, ngraham
Cc: abetts, davidedmundson, plasma-devel, jraleigh, GB_2, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D20863: Allow folder view elements to be be dropped using other Action than Copy

[Nathaniel Graham]

ngraham added a comment.


  Now don't forget to merge to master. :)
  
git checkout Plasma/5.15
git pull
git checkout master
git merge -s recursive -Xours origin/Plasma/5.15
git push
  
  
  
  ---
  
  I have a handy shell function `kmerge` that does this for me and then checks 
the result:
  
function kmerge {
# Make sure the merge branch is up to date
CURRENTBRANCH=`git branch 2> /dev/null | sed -e '/^[^*]/d' -e 's/* 
\(.*\)/\1/'`
git checkout $1
git pull

# Do the merge safely
git checkout $CURRENTBRANCH
git merge -s recursive -Xours $1

# Verify it afterwards
git diff origin/HEAD HEAD

# Now push!
}

REPOSITORY
  R119 Plasma Desktop

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

To: meven, ngraham, hein
Cc: hein, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D20863: Allow folder view elements to be be dropped using other Action than Copy

[Méven Car]

This revision was automatically updated to reflect the committed changes.
Closed by commit R119:b753301b74ac: Allow folder view elements to be be dropped 
using other Action than Copy (authored by meven).

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D20863?vs=57092=57608

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

AFFECTED FILES
  containments/desktop/plugins/folder/foldermodel.cpp
  containments/desktop/plugins/folder/foldermodel.h

To: meven, ngraham, hein
Cc: hein, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D21027: [Notes] Fix icons being almost invisible with light backgrounds

[Filip Fila]

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


  In D21027#461230 , @ngraham wrote:
  
  > Gotcha. At least use a function instead of duplicating the same code in 
each button.
  
  
  Ah yeah, rereading your original comment I now see that bit went over my 
head, that would be much better so I will make those changes.
  
  >> The other thing about components is that Plasma Components doesn't support 
`icon.color`. I'd have kept using it, but PC2 spews out an error, while with 
PC3 it didn't complain but didn't work anyway.
  > 
  > OK, that makes sense. Can you make it a bit more clear in the Description?
  
  Yep!

REPOSITORY
  R114 Plasma Addons

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

To: filipf, #plasma, #vdg, ngraham
Cc: broulik, davidedmundson, plasma-devel, jraleigh, GB_2, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D21027: [Notes] Fix icons being almost invisible with light backgrounds

[Nathaniel Graham]

ngraham added a comment.


  In D21027#461027 , @filipf wrote:
  
  > I get what you're saying but that's just how the applet is designed. It's 
preconfigured SVG backgrounds (you can find them in 
`/usr/share/plasma/desktoptheme/default/widgets/notes.svgz`). I'm not really 
introducing any hardcoding that isn't already there. See the hardcoding of text 
color:
  >
  >   style: PlasmaStyle.TextAreaStyle {
  >   //this is deliberately _NOT_ the theme color as we are over a known 
bright background
  >   //an unknown colour over a known colour is a bad move as you end up 
with white on yellow
  >   textColor: (plasmoid.configuration.color === "black" || 
plasmoid.configuration.color === "translucent-light") ? "#dfdfdf" : "#202020"
  >   }
  >   
  >
  > So one thing is if we'd want to rewrite the whole thing to be a rectangle 
and then you could select a huge amount of colors as the background I guess, as 
well as have a spinbox for opacity. Also tagging @broulik as he might know 
better why it's not like that in the first place.
  
  
  Gotcha. At least use a function instead of duplicating the same code in each 
button.
  
  > The other thing about components is that Plasma Components doesn't support 
`icon.color`. I'd have kept using it, but PC2 spews out an error, while with 
PC3 it didn't complain but didn't work anyway.
  
  OK, that makes sense. Can you make it a bit more clear in the Description?

REPOSITORY
  R114 Plasma Addons

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

To: filipf, #plasma, #vdg, ngraham
Cc: broulik, davidedmundson, plasma-devel, jraleigh, GB_2, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D20863: Allow folder view elements to be be dropped using other Action than Copy

[Nathaniel Graham]

ngraham added a comment.


  Please land this on the stable branch (`Plasma/5.15`) before tagging in two 
days, then merge to master. See 
https://community.kde.org/Infrastructure/Phabricator#Landing_on_the_.22Stable_branch.22
  
  If you need help, don't hesitate to let me know!

REPOSITORY
  R119 Plasma Desktop

BRANCH
  master

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

To: meven, ngraham, hein
Cc: hein, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D20863: Allow folder view elements to be be dropped using other Action than Copy

[Méven Car]

meven added a comment.


  @hein is it ok ?

REPOSITORY
  R119 Plasma Desktop

BRANCH
  master

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

To: meven, ngraham, hein
Cc: hein, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D21035: Remove disconnected network devices from Network Monitor.

[Bernhard Übelacker]

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

REVISION SUMMARY
  USB network devices or VirtualBox bridges are just
  temporarily up and active in the system.
  
  The Network Monitor Widget automatically adds such interfaces
  when such devices get connected.
  Unfortunately it does not remove them when disconnected.
  
  This patch tries to implement also the removal and
  works in current Debian testing/buster.
  
  But it touches the common file Applet.qml, so it may have unwanted
  side effects to other widgets in its current state.
  
  Betriebssystem: Debian GNU/Linux
  KDE-Plasma-Version: 5.14.5
  Qt-Version: 5.11.3
  KDE-Frameworks-Version: 5.54.0
  Kernel-Version: 4.19.0-4-amd64
  Art des Betriebssystems: 64-bit
  Prozessoren: 16 × AMD Ryzen 7 1700 Eight-Core Processor
  Speicher: 15,5 GiB Arbeitsspeicher

REPOSITORY
  R120 Plasma Workspace

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

AFFECTED FILES
  applets/systemmonitor/common/contents/ui/Applet.qml
  applets/systemmonitor/net/contents/ui/net.qml

To: bernhardu
Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D21033: Panels: reset shadow pixmaps on theme change

[Friedrich W. H. Kossebau]

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

REPOSITORY
  R120 Plasma Workspace

BRANCH
  resetshadowpixmapsonthemechange

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

AFFECTED FILES
  shell/panelshadows.cpp

To: kossebau, #plasma
Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D21030: [WidgetExplorer] Fix blurry previews

[Filip Fila]

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

REVISION SUMMARY
  Round the cell width to avoid getting blurry previews.
  
  NOTE: It also seems further improvements could be done for the icons 
themselves (advice, tips?)

REPOSITORY
  R119 Plasma Desktop

BRANCH
  no-blurry-previews-widget-explorer (branched from master)

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

AFFECTED FILES
  desktoppackage/contents/explorer/WidgetExplorer.qml

To: filipf
Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D20265: Introduce libnotificationmanager

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> davidedmundson wrote in jobsmodel_p.cpp:358
> you're not filtering the jobs based on the desktopEntry
> 
> every app gets notified about anyones percentage?

Good catch, forgot a check for `desktopEntry` there

> hein wrote in notificationgroupingproxymodel.cpp:38
> Careful there - these will match when both are empty, which is why the libtm 
> version tests for that. Are you sure this data cannot ever be missing?

I'm pretty sure it can never be empty as if all else fails I use the binary 
name of the process that sent the notification dbus message (which might fail 
after all?), I'll add a check nonetheless.

> hein wrote in notifications.cpp:828
> While this isn't an objection, I usually use QMetaEnum to compute this from 
> the enum instead of having a block of stuff that needs to be synced.

There's a gazillion places that need to be synced when we add something:

- add enum in header
- add new field to struct that holds the actual data
- let `data()` return the new role

as well as actually forwarding it in a bunch of places in QML, so I don't think 
automating this particular part really reduces the workload for that. I can 
give it a shot, though.

> hein wrote in notifications.h:211
> I recommend making this Q_ENUM and registering it; with libtm it turned out 
> that it's useful to be able to call data() with a role from QML sometimes.

I did exactly that in a later commit that isn't on Phab, sorry :)

> hein wrote in notifications.h:217
> This is a bit unusual, I assume it's because you're flattening back out so 
> you don't want rowCount/canFetchMore?

I guess. I use it for showing an expand button with "show n more" written on it

REPOSITORY
  R120 Plasma Workspace

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

To: broulik, #plasma
Cc: hein, mart, nicolasfella, davidedmundson, ngraham, plasma-devel, jraleigh, 
GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol

D20265: Introduce libnotificationmanager

[Eike Hein]

hein added inline comments.

INLINE COMMENTS

> notificationgroupcollapsingproxymodel.cpp:175
> +emit dataChanged(idx, idx, dirtyRoles);
> +emit dataChanged(idx.child(0, 0), idx.child(rowCount(idx) - 1, 0), 
> dirtyRoles);
> +

I know this really sucks, and I'm not going to be pushy on this because you've 
been waiting on this review forever.

But: Qt 5.13 has deprecated `QModelIndex::child` (they want us to use 
`QAIM::index` now always), leading to noisy builds. For new code it'd be 
amazing to avoid it.

REPOSITORY
  R120 Plasma Workspace

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

To: broulik, #plasma
Cc: hein, mart, nicolasfella, davidedmundson, ngraham, plasma-devel, jraleigh, 
GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol

D20265: Introduce libnotificationmanager

[Eike Hein]

hein added inline comments.

INLINE COMMENTS

> notificationgroupcollapsingproxymodel.cpp:52
> +Q_UNUSED(bottomRight); // what about it?
> +Q_UNUSED(roles);
> +

Not actually unused.

`bottomRight`: You sure the source model will emit for single cells? Otherwise 
you need to loop over the range.

REPOSITORY
  R120 Plasma Workspace

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

To: broulik, #plasma
Cc: hein, mart, nicolasfella, davidedmundson, ngraham, plasma-devel, jraleigh, 
GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol

D20265: Introduce libnotificationmanager

[Eike Hein]

hein added inline comments.

INLINE COMMENTS

> notificationgroupingproxymodel.cpp:38
> +
> +bool NotificationGroupingProxyModel::appsMatch(const QModelIndex , const 
> QModelIndex ) const
> +{

Careful there - these will match when both are empty, which is why the libtm 
version tests for that. Are you sure this data cannot ever be missing?

REPOSITORY
  R120 Plasma Workspace

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

To: broulik, #plasma
Cc: hein, mart, nicolasfella, davidedmundson, ngraham, plasma-devel, jraleigh, 
GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol

D20265: Introduce libnotificationmanager

[Eike Hein]

hein added inline comments.

INLINE COMMENTS

> notificationsmodel.cpp:57
> +
> +int rowOfNotification(uint id) const;
> +

Could be worth making `inline` maybe?

REPOSITORY
  R120 Plasma Workspace

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

To: broulik, #plasma
Cc: hein, mart, nicolasfella, davidedmundson, ngraham, plasma-devel, jraleigh, 
GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol

D20265: Introduce libnotificationmanager

[Eike Hein]

hein added inline comments.

INLINE COMMENTS

> notifications.cpp:828
> +{
> +return QHash {
> +{IdRole, QByteArrayLiteral("notificationId")}, // id is QML-reserved

While this isn't an objection, I usually use QMetaEnum to compute this from the 
enum instead of having a block of stuff that needs to be synced.

> notifications.h:211
> +
> +enum Roles {
> +IdRole = Qt::UserRole + 1, ///< A notification identifier. This can 
> be uint notification ID or string application job source.

I recommend making this Q_ENUM and registering it; with libtm it turned out 
that it's useful to be able to call data() with a role from QML sometimes.

REPOSITORY
  R120 Plasma Workspace

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

To: broulik, #plasma
Cc: hein, mart, nicolasfella, davidedmundson, ngraham, plasma-devel, jraleigh, 
GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol

D21029: UpdatesPage: Don't try to translate version numbers

[Pino Toscano]

pino added a comment.


  The explanation of this change does not match the code changes: the code 
never translated the version numbers, but it composed a string showing the 
version numbers before and after the upgrade.
  Could it simply be that one of the two (for example `installedVersion`) is 
null?

INLINE COMMENTS

> UpdatesPage.qml:239
>  elide: Text.ElideRight
> -text: i18n("%1 → %2", installedVersion, 
> availableVersion)
> +text: "%1 → 
> %2".arg(installedVersion).arg(availableVersion)
>  visible: !truncated

I do not think this change is correct: "%1 → %2" is a UI string, and thus it 
must be translatable.

REPOSITORY
  R134 Discover Software Store

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

To: jbbgameich, #discover_software_store
Cc: pino, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D21029: UpdatesPage: Don't try to translate version numbers

[Jonah Brüchert]

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

REVISION SUMMARY
  Fixes a I18N_ARGUMENT_MISSING warning in the ui that appears sometimes while 
discover is looking for updates.

TEST PLAN
  Version numbers are still displayed, and no warnings are visible

REPOSITORY
  R134 Discover Software Store

BRANCH
  updates-page-i18n-versions (branched from master)

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

AFFECTED FILES
  discover/qml/UpdatesPage.qml

To: jbbgameich
Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D20265: Introduce libnotificationmanager

[Eike Hein]

hein added inline comments.

INLINE COMMENTS

> notifications.h:217
> +IsGroupRole = Qt::UserRole + 2, ///< Whether the item is a group
> +GroupChildrenCountRole, ///< The number of children in a group.
> +ExpandedGroupChildrenCountRole, ///< The number of children in a 
> group that are expanded.

This is a bit unusual, I assume it's because you're flattening back out so you 
don't want rowCount/canFetchMore?

REPOSITORY
  R120 Plasma Workspace

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

To: broulik, #plasma
Cc: hein, mart, nicolasfella, davidedmundson, ngraham, plasma-devel, jraleigh, 
GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol

D21027: [Notes] Fix icons being almost invisible with light backgrounds

[Filip Fila]

filipf added a subscriber: broulik.
filipf added a comment.


  I get what you're saying but that's just how the applet is designed. It's 
preconfigured SVG backgrounds (you can find them in 
`/usr/share/plasma/desktoptheme/default/widgets/notes.svgz`). I'm not really 
introducing any hardcoding that isn't already there. See the hardcoding of text 
color:
  
style: PlasmaStyle.TextAreaStyle {
//this is deliberately _NOT_ the theme color as we are over a known 
bright background
//an unknown colour over a known colour is a bad move as you end up 
with white on yellow
textColor: (plasmoid.configuration.color === "black" || 
plasmoid.configuration.color === "translucent-light") ? "#dfdfdf" : "#202020"
}
  
  So one thing is if we'd want to rewrite the whole thing to be a rectangle and 
then you could select a huge amount of colors as the background I guess, as 
well as have a spinbox for opacity. Also tagging @broulik as he might know 
better why it's not like that in the first place.
  
  The other thing about components is that Plasma Components doesn't support 
`icon.color`. I'd have kept using it, but PC2 spews out an error, while with 
PC3 it didn't complain but didn't work anyway.

REPOSITORY
  R114 Plasma Addons

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

To: filipf, #plasma, #vdg, ngraham
Cc: broulik, davidedmundson, plasma-devel, jraleigh, GB_2, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D20859: Crash in sddmthemeinstaller invalid use of errorString

[John Gehrig]

johngehrig added a comment.


  Sorry for the delay... I was finally able to get the crashing machine set up 
again.
  
  Yes, it looks like the descuctor for `job` was called while the KMessageBox 
was in the process of being painted onscreen (breakpoint in 
kauthexecutejob.cpp).
  
  I tried making `job` a `QPointer` and QSharedPointer with 
the original qWarning/KMessageBox lines, but that didn't help. I can't say I'm 
familiar enough with the intricacies of how KDE/Qt handles object allocations 
and events to explain...

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

To: johngehrig, ngraham, davidedmundson, #plasma
Cc: anthonyfieroni, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D20859: Crash in sddmthemeinstaller invalid use of errorString

[John Gehrig]

johngehrig updated this revision to Diff 57581.
johngehrig edited the summary of this revision.
johngehrig edited the test plan for this revision.
johngehrig added a comment.


  Ordering changed, qWarning first.
  job->errorString() displayed for all values of job->error().
  A QString object was created, so job is no longer passed to KMessageBox.

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D20859?vs=57081=57581

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

AFFECTED FILES
  sddmthemeinstaller.cpp

To: johngehrig, ngraham, davidedmundson, #plasma
Cc: anthonyfieroni, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart