Review Request 128682: Fix typo in icon name

[Fabian Vogt]

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

Review request for Plasma.


Repository: plasma-pk-updates


Description
---

Introduced in commit 9e236ec5d4d28d7498dfcc830a95590dececcf02


Diffs
-

  src/declarative/pkupdates.cpp 324387f 

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


Testing
---

Built and run, icon appears fine.


Thanks,

Fabian Vogt

Re: Review Request 128682: Fix typo in icon name

[Fabian Vogt]

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

(Updated Aug. 15, 2016, 10:40 a.m.)


Status
--

This change has been marked as submitted.


Review request for Plasma.


Changes
---

Submitted with commit 9662b9c5d83ad0188803830d7cf9a96a2db0d114 by David 
Edmundson on behalf of Fabian Vogt to branch master.


Repository: plasma-pk-updates


Description
---

Introduced in commit 9e236ec5d4d28d7498dfcc830a95590dececcf02


Diffs
-

  src/declarative/pkupdates.cpp 324387f 

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


Testing
---

Built and run, icon appears fine.


Thanks,

Fabian Vogt

Re: Review Request 128815: Fix themes list with SDDM 0.14

[Fabian Vogt]

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



IMO you should use the same method as sddm, which is hardcoded at configure 
time: "@DATA_INSTALL_DIR@/themes".

- Fabian Vogt


On Sept. 2, 2016, 5:51 nachm., Antonio Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128815/
> ---
> 
> (Updated Sept. 2, 2016, 5:51 nachm.)
> 
> 
> Review request for Plasma and David Edmundson.
> 
> 
> Repository: sddm-kcm
> 
> 
> Description
> ---
> 
> SDDM 0.14 apparently creates a ~/.local/share/sddm dir to store some logs, 
> which makes sddm-kcm look for themes there instead of /usr/share/sddm
> Additionally, fix the config key for themes dir in sddm.conf
> 
> 
> Diffs
> -
> 
>   src/themesmodel.cpp 5af9c54 
> 
> Diff: https://git.reviewboard.kde.org/r/128815/diff/
> 
> 
> Testing
> ---
> 
> Themes list works again with SDDM 0.14
> 
> 
> Thanks,
> 
> Antonio Rojas
> 
>

Re: Review Request 129404: [AppletInterface] Never pull focus away from fullRepresentation

[Fabian Vogt]

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


Ship it!




Tested, works fine!

- Fabian Vogt


On Nov. 14, 2016, 4:25 nachm., Kai Uwe Broulik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129404/
> ---
> 
> (Updated Nov. 14, 2016, 4:25 nachm.)
> 
> 
> Review request for Plasma and Fabian Vogt.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> When closing the launcher using Meta key, it pulls focus away and when 
> clicking the button to expand the applet, it won't get focus.
> 
> BUG: 372476
> 
> 
> Diffs
> -
> 
>   src/scriptengines/qml/plasmoid/appletinterface.cpp f24bc51 
> 
> Diff: https://git.reviewboard.kde.org/r/129404/diff/
> 
> 
> Testing
> ---
> 
> started plasmashell:
> 
> click on kickoff -> it opened, typing searched
> click on kickoff -> it closed
> pressed meta -> it opened, typing searched
> pressed meta -> it closed
> click on kickoff -> it opened, typing searched (before it wouldnt search 
> until I click the window)
> 
> 
> Thanks,
> 
> Kai Uwe Broulik
> 
>

[Differential] [Request, 3 lines] D4464: Add missing include in sortedactivitiesmodel to fix build with GCC 7I did not build-test this, but it's fairly obvious.

[Fabian Vogt]

fvogt created this revision.
fvogt added a reviewer: Plasma.
fvogt added a subscriber: Plasma.
Restricted Application added a project: Plasma.
Restricted Application added a subscriber: plasma-devel.

REVISION SUMMARY
  Fixes "error: 'placeholders' is not a namespace-name"

REPOSITORY
  R119 Plasma Desktop

BRANCH
  Plasma/5.9

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

AFFECTED FILES
  imports/activitymanager/sortedactivitiesmodel.cpp

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: fvogt, #plasma
Cc: plasma-devel, #plasma, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol

[Differential] [Closed] D4464: Add missing include in sortedactivitiesmodel to fix build with GCC 7I did not build-test this, but it's fairly obvious.

[Fabian Vogt]

fvogt closed this revision.

REPOSITORY
  R119 Plasma Desktop

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: fvogt, #plasma, davidedmundson
Cc: plasma-devel, #plasma, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol

[Differential] [Request, 2 lines] D4579: Do not treat filename in selection as URL

[Fabian Vogt]

fvogt created this revision.
Restricted Application added a project: Plasma.
Restricted Application added a subscriber: plasma-devel.

REVISION SUMMARY
  KFileWidget::setSelection(QString &) accepts either absolute URLs
  or relative paths. If the filename contains a :, it gets treated
  as a URL and gets rejected. This forces setSelection to parse
  it as URL.
  
  CCBUG: 376365

REPOSITORY
  R135 Integration for Qt applications in Plasma

BRANCH
  master

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

AFFECTED FILES
  src/platformtheme/kdeplatformfiledialoghelper.cpp

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: fvogt
Cc: plasma-devel, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

[Differential] [Updated] D4579: Do not treat filename in selection as URL

[Fabian Vogt]

fvogt added a reviewer: Plasma.
fvogt added a comment.


  https://bugs.kde.org/show_bug.cgi?id=376365 contains reproducer, with this 
change it works.

REPOSITORY
  R135 Integration for Qt applications in Plasma

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: fvogt, #plasma
Cc: plasma-devel, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

[Differential] [Closed] D4579: Do not treat filename in selection as URL

[Fabian Vogt]

This revision was automatically updated to reflect the committed changes.
Closed by commit R135:e70f8134a2bc: Do not treat filename in selection as URL 
(authored by fvogt).

REPOSITORY
  R135 Integration for Qt applications in Plasma

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D4579?vs=11240&id=11294

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

AFFECTED FILES
  src/platformtheme/kdeplatformfiledialoghelper.cpp

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: fvogt, #plasma, mart
Cc: plasma-devel, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

[Differential] [Accepted] D4654: Port to QMultiHash.

[Fabian Vogt]

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


  If all cases of insert(key,value) are validated to behave correctly (same as 
insertMulti now!), this looks good to me.
  
  Easier to understand and fewer code lines!

REPOSITORY
  R120 Plasma Workspace

BRANCH
  Plasma/5.8

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: hein, #plasma, graesslin, fvogt
Cc: plasma-devel, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

[Differential] [Request, 4 lines] D4794: Fix default fixed font in fonts kcm

[Fabian Vogt]

fvogt created this revision.
Restricted Application added a project: Plasma.
Restricted Application added a subscriber: plasma-devel.

REVISION SUMMARY
  Now it is the same as used by plasma-integration.
  Also adjust the file name in the comment, which was apparently forgotten.

TEST PLAN
  I didn't test it yet...

REPOSITORY
  R119 Plasma Desktop

BRANCH
  Plasma/5.8

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

AFFECTED FILES
  kcms/fonts/fonts.cpp

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: fvogt
Cc: plasma-devel, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol

[Differential] [Updated] D4794: Fix default fixed font in fonts kcm

[Fabian Vogt]

fvogt added a reviewer: Plasma.

REPOSITORY
  R119 Plasma Desktop

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: fvogt, #plasma
Cc: plasma-devel, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol

[Differential] [Updated] D4794: Fix default fixed font in fonts kcm

[Fabian Vogt]

fvogt edited the test plan for this revision.

REPOSITORY
  R119 Plasma Desktop

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: fvogt, #plasma
Cc: plasma-devel, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol

[Differential] [Closed] D4794: Fix default fixed font in fonts kcm

[Fabian Vogt]

This revision was automatically updated to reflect the committed changes.
Closed by commit R119:d02de0db36a3: Fix default fixed font in fonts kcm 
(authored by fvogt).

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D4794?vs=11826&id=11841

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

AFFECTED FILES
  kcms/fonts/fonts.cpp

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: fvogt, #plasma, davidedmundson
Cc: plasma-devel, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol

[Differential] [Request, 24 lines] D4821: Implement manual focus on click

[Fabian Vogt]

fvogt created this revision.
Restricted Application added a project: Plasma.

REVISION SUMMARY
  Currently only the first created screenlock window gets focus.
  On clicks, no focus events are sent, which makes it impossible to input
  passwords. This patch now makes it possible to focus to a different
  screenlock window (on a different monitor, for example) using a mouse
  button press.
  This should also fix newly created screenlock windows stealing the focus
  of already displayed ones as only the first window gains automatic focus.
  
  BUG: 348789
  BUG: 374289

TEST PLAN
  Locked the screen, now I can use the password input on the secondary screen
  as well.

REPOSITORY
  R133 KScreenLocker

BRANCH
  Plasma/5.8

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

AFFECTED FILES
  x11locker.cpp
  x11locker.h

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: fvogt, plasma-devel, graesslin, broulik
Cc: plasma-devel, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol

[Differential] [Commented On] D4821: Implement manual focus on click

[Fabian Vogt]

fvogt added a comment.


  This is weird: While the fix works absolutely fine (and reliably) for me on 
my primary user account,
  it does not have any effect for a different user around and @broulik
  
  Even weirder: I cannot reproduce the original bug in any of my VMs using 
multihead QXL...
  
  So while it does improve the situation I won't push this without further 
investigation for now.

REPOSITORY
  R133 KScreenLocker

BRANCH
  Plasma/5.8

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: fvogt, graesslin, broulik, #plasma
Cc: plasma-devel, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol

[Differential] [Commented On] D4821: Implement manual focus on click

[Fabian Vogt]

fvogt added a comment.


  In https://phabricator.kde.org/D4821#90657, @graesslin wrote:
  
  > Just as an fyi: in the build tree there is a test app which does the 
complete locking without the need for restart the session.
  
  
  Would that really work for changes in x11locker.cpp as well? it's part of 
libkscreenlocker, linked to ksmserver.
  So the only way to test it would be a nested/virtual X server with a separate 
ksmserver, wouldn't it?
  As far as I can tell, kscreenlocker_test (if that's what you mean) only tests 
the locker and not the ksmserver part.
  
  > Overall I think that the problem might also be in the greeter.
  
  It works fine with kscreenlocker_greet --testing
  
  > On Wayland we have the same issue, which could indicate a problem in the 
greeter as well.
  
  Multiscreen on wayland is borked on my system for some reason, so I can't 
test that...

REPOSITORY
  R133 KScreenLocker

BRANCH
  Plasma/5.8

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: fvogt, graesslin, broulik, #plasma
Cc: plasma-devel, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol

[Differential] [Updated, 28 lines] D4821: Implement manual focus on click

[Fabian Vogt]

fvogt updated this revision to Diff 11916.
fvogt added a comment.


  Handle the case better when the currently focused window vanishes

REPOSITORY
  R133 KScreenLocker

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D4821?vs=11898&id=11916

BRANCH
  Plasma/5.8

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

AFFECTED FILES
  x11locker.cpp
  x11locker.h

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: fvogt, graesslin, broulik, #plasma
Cc: plasma-devel, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol

[Differential] [Commented On] D4821: Implement manual focus on click

[Fabian Vogt]

fvogt added a comment.


  I finally figured the issue out.
  Newly appearing windows try to steal focus, which makes m_focusedLockWindow 
actually wrong.
  That explains why it works only for some and only for some setups...
  I'll try to fix that by adding
  
setAttribute(Qt::WA_ShowWithoutActivating);
  
  to the greeter windows.
  Now the question is whether that would break the wayland locker as there is 
no call to fakeFocusIn on wayland.
  Either that code needs to be X11 only or the focus handling fixed under 
wayland as well, which I cannot do as I
  cannot get multiscreen on wayland to work...

REPOSITORY
  R133 KScreenLocker

BRANCH
  Plasma/5.8

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: fvogt, graesslin, broulik, #plasma
Cc: plasma-devel, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol

[Differential] [Updated, 29 lines] D4821: Implement manual focus on click

[Fabian Vogt]

fvogt updated this revision to Diff 11941.
fvogt added a comment.


  Newly created windows do not steal focus anymore
  
  Needs a quick test that it does not break wayland (e.g. no focus at all)

REPOSITORY
  R133 KScreenLocker

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D4821?vs=11916&id=11941

BRANCH
  Plasma/5.8

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

AFFECTED FILES
  greeter/greeterapp.cpp
  x11locker.cpp
  x11locker.h

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: fvogt, graesslin, broulik, #plasma
Cc: plasma-devel, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol

[Differential] [Requested Review] D4821: Implement manual focus on click

[Fabian Vogt]

fvogt requested review of this revision.

REPOSITORY
  R133 KScreenLocker

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: fvogt, broulik, #plasma, graesslin
Cc: plasma-devel, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol

[Differential] [Commented On] D4821: Implement manual focus on click

[Fabian Vogt]

fvogt added a comment.


  In https://phabricator.kde.org/D4821#91393, @hein wrote:
  
  > > On Wayland screen locker is initially focused but clicking on different 
screens doesn't focus them.
  >
  > If this is outstanding, maybe add a FIXME TODO so we don't forget ...?
  
  
  Yes, but that can only be fixed in kwin (?)
  
  The right approach to finally fix this issue (and also get rid of a lot of 
code) would be to get the shared typing working again, which likely needs a 
total rewrite for Qt Quick 2.

REPOSITORY
  R133 KScreenLocker

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: fvogt, #plasma, graesslin, broulik
Cc: hein, plasma-devel, progwolff, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol

[Differential] [Closed] D4821: Implement manual focus on click

[Fabian Vogt]

This revision was automatically updated to reflect the committed changes.
Closed by commit R133:f8043de10b5d: Implement manual focus on click (authored 
by fvogt).

REPOSITORY
  R133 KScreenLocker

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D4821?vs=11941&id=12030

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

AFFECTED FILES
  greeter/greeterapp.cpp
  x11locker.cpp
  x11locker.h

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: fvogt, #plasma, graesslin, broulik
Cc: hein, plasma-devel, progwolff, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol

D4955: [QDBusMenuBar] Connect to popupRequested signal

[Fabian Vogt]

fvogt requested changes to this revision.
fvogt added a comment.
This revision now requires changes to proceed.


  Can't apply to Plasma/5.9 with arc patch as your diff is missing the 
workaround code for Qt 5.7.0.

REPOSITORY
  R135 Integration for Qt applications in Plasma

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

To: broulik, #plasma, davidedmundson, fvogt
Cc: plasma-devel, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol

D4955: [QDBusMenuBar] Connect to popupRequested signal

[Fabian Vogt]

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


  Tested this with Qt 5.6.2 + backports, the signal gets triggered fine. So far 
it only seems to work with a few select apps (dolphin), some (e.g. konsole) do 
not assign shortcuts at all and others add shortcuts but assign them more than 
once...

REPOSITORY
  R135 Integration for Qt applications in Plasma

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

To: broulik, #plasma, davidedmundson, fvogt
Cc: plasma-devel, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol

D5025: Do not use fixed steps when scrolling on battery icon

[Fabian Vogt]

fvogt created this revision.
Restricted Application added a project: Plasma.

REVISION SUMMARY
  This makes it possible to have some more fine-grained control over
  the screen brightness with scroll devices that support it, like
  touchpads or trackpoints.

TEST PLAN
  Tested with touchpad, trackpoint and scroll wheel on mouse

REPOSITORY
  R120 Plasma Workspace

BRANCH
  batteryscroll

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

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

To: fvogt, #plasma
Cc: plasma-devel, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol

D5025: Do not use fixed steps when scrolling on battery icon

[Fabian Vogt]

fvogt added a comment.


  Arc seems to have selected my local branch as target branch for some reason, 
it should be "master" instead, obviously

REPOSITORY
  R120 Plasma Workspace

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

To: fvogt, #plasma
Cc: plasma-devel, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol

D5025: Do not use fixed steps when scrolling on battery icon

[Fabian Vogt]

This revision was automatically updated to reflect the committed changes.
Closed by commit R120:82a44a3f264c: Do not use fixed steps when scrolling on 
battery icon (authored by fvogt).

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D5025?vs=12405&id=12478

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

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

To: fvogt, #plasma, broulik
Cc: plasma-devel, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol

D5227: Add a configuration option to hide the show password button

[Fabian Vogt]

fvogt created this revision.
Restricted Application added a project: Plasma.

REVISION SUMMARY
  The show password button only makes sense with a virtual keyboard
  on touchscreens. On desktop PCs it's not only not useful, but it
  can also be used to show (partially) typed in passwords.
  It also makes it possible to display clipboard entries in plaintext,
  which is why the clipboard is cleared when the screen locks.
  By adding a configuration option, the user can decide whether the
  show password functionality is useful to him.
  The default is set to show the button to now change the default
  behaviour.
  
  Once the clipboard issue is fixed properly in all cases, the configuration
  option can be reevaluated, but currently it is in my opinion the best
  way to fix both issues at once.

TEST PLAN
  Ran kscreenlocker_greet --testing with and without the option
  changed in the kcm. Worked as expected.

REPOSITORY
  R133 KScreenLocker

BRANCH
  master

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

AFFECTED FILES
  greeter/greeterapp.cpp
  kcfg/kscreenlockersettings.kcfg
  kcm/kcm.ui

To: fvogt, #plasma, graesslin
Cc: plasma-devel, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol

D5227: Add a configuration option to hide the show password button

[Fabian Vogt]

fvogt updated this revision to Diff 12933.
fvogt added a comment.


  Add a comment in greeterapp.cpp and also add a tooltip to the configuration 
option label.

REPOSITORY
  R133 KScreenLocker

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D5227?vs=12932&id=12933

BRANCH
  master

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

AFFECTED FILES
  greeter/greeterapp.cpp
  kcfg/kscreenlockersettings.kcfg
  kcm/kcm.ui

To: fvogt, #plasma, graesslin
Cc: plasma-devel, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol

D5227: Add a configuration option to hide the show password button

[Fabian Vogt]

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


  I would fix the clipboard content issue with klipper (although I find that 
approach wrong as it's too complicated as a seemingly unrelated component needs 
to support the screenlocker), but I do not have enough experience in that area 
to do it properly.
  
  Once that is fixed, I can remove the condition around the clipboard clear 
code. I would very much like to keep the show password button configuration 
option.
  
  > It's the typical KDE 3 approach of not knowing what we want to do, so we 
offer an option.
  
  No, it's the approach of setting a sane default and letting the user change 
it.

REPOSITORY
  R133 KScreenLocker

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

To: fvogt, #plasma, graesslin
Cc: plasma-devel, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol

D5227: Add a configuration option to hide the show password button

[Fabian Vogt]

fvogt added a comment.


  In https://phabricator.kde.org/D5227#98542, @broulik wrote:
  
  > For show password button there's a kiosk restriction available: 
**lineedit_reveal_password** (see 
https://userbase.kde.org/KDE_System_Administration/Kiosk/Keys)
  
  
  Indeed, that works! Now if just there was a user-friendly way to configure 
it...
  
  IMO kiosk is not really the right place for that and it's still useful for 
sharing shared passwords, like for WiFi.

REPOSITORY
  R133 KScreenLocker

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

To: fvogt, #plasma, graesslin
Cc: broulik, plasma-devel, progwolff, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol

D5227: Add a configuration option to hide the show password button

[Fabian Vogt]

fvogt added a comment.


  In https://phabricator.kde.org/D5227#98555, @graesslin wrote:
  
  > I'm against adding this option as the greeter does not have any influence 
over the lnf package. It works with our default but as soon as users change the 
theme we do not know. I'm against adding new options which only work in the 
default setup.
  
  
  Understandably. Is there a way for look-and-feels to offer own configuration 
options?

REPOSITORY
  R133 KScreenLocker

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

To: fvogt, #plasma, graesslin
Cc: broulik, plasma-devel, progwolff, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol

D5351: kcm_baloofile: Add option to disable file content indexing

[Fabian Vogt]

fvogt created this revision.
Restricted Application added a project: Plasma.

REVISION SUMMARY
  Baloo supports "only basic indexing" since version 5.15, which
  causes it to only store file names into the database:
  https://community.kde.org/Baloo/Configuration

TEST PLAN
  Ran "balooctl config show contentIndexing" after changing the option.

REPOSITORY
  R119 Plasma Desktop

BRANCH
  Plasma/5.9

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

AFFECTED FILES
  CMakeLists.txt
  kcms/baloo/configwidget.ui
  kcms/baloo/kcm.cpp
  kcms/baloo/kcm.h

To: fvogt, #plasma
Cc: plasma-devel, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol

D5351: kcm_baloofile: Add option to disable file content indexing

[Fabian Vogt]

fvogt updated this revision to Diff 13233.
fvogt added a comment.


  Obviously for master only.

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D5351?vs=13232&id=13233

BRANCH
  master

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

AFFECTED FILES
  CMakeLists.txt
  kcms/baloo/configwidget.ui
  kcms/baloo/kcm.cpp
  kcms/baloo/kcm.h

To: fvogt, #plasma
Cc: plasma-devel, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol

D12405: [WIP] Per-screen scale factors on X11 using QT_SCREEN_SCALE_FACTORS

[Fabian Vogt]

fvogt added a comment.


  In D12405#676733 , @memeplex wrote:
  
  > So I set QT_SCREEN_SCALE_FACTORS='eDP-1=2;DP-2=1.7;'. But consider:
  >
  > 1. QT_AUTO_SCREEN_SCALE_FACTOR=1 
QT_SCREEN_SCALE_FACTORS='eDP-1=2;DP-2=1.7;' konsole
  > 2. QT_AUTO_SCREEN_SCALE_FACTOR=0 
QT_SCREEN_SCALE_FACTORS='eDP-1=2;DP-2=1.7;' konsole
  >
  >   The first one correctly reduces the size of konsole (menubar, text inside 
the terminal emulator). The second one does nothing at all.
  >
  >   So since plasma seems to be deliveratelt setting 
QT_AUTO_SCREEN_SCALE_FACTOR=0, I don't see how this would possibly work.
  
  
  It should work. Are you sure the screen names are correct and match? You can 
run `QT_LOGGING_RULES=qt.qpa.screen.debug=true konsole` to get the ones 
`konsole` itself sees.

REPOSITORY
  R104 KScreen

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

To: fvogt, #plasma
Cc: memeplex, anthonyfieroni, luxus, snugghash, gladhorn, mart, hein, ngraham, 
graesslin, davidedmundson, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
cblack, jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra

D12405: [WIP] Per-screen scale factors on X11 using QT_SCREEN_SCALE_FACTORS

[Fabian Vogt]

fvogt added a comment.


  I just tried that here and it works fine (Qt 5.15.1). Maybe there's something 
else in the environment which breaks it?

REPOSITORY
  R104 KScreen

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

To: fvogt, #plasma
Cc: memeplex, anthonyfieroni, luxus, snugghash, gladhorn, mart, hein, ngraham, 
graesslin, davidedmundson, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
cblack, jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra

Re: Relicensing Plasma Browser Integration Host to GPLv3+

[Fabian Vogt]

Moin,

Am Dienstag, 5. Januar 2021, 23:29:38 CET schrieb Kai Uwe Broulik:
> Hi all,
> 
> I've noticed that the licensing of Plasma Browser Integration is all 
> over the place, some parts seem to be resembling an MIT-style license, 
> probably carried over all the way from when it was just a proof of 
> concept we didn't care much about.
> 
> To clean up that mess, I'd like to relicense the host (all code in 
> plasma-browser-integration/host) to GPLv3+ to match the newer host code 
> and extension code.
> 
> According to git log the following people (and myself) touched that folder:
> 
> David Edmundson
> Fabian Vogt
> Laurent Montel
> Friedrich W. H. Kossebau
> Matthijs Tijink
> 
> Are you fine with this, or have any other comments?

I'm fine with GPL-3.0-or-later, yes.

Cheers,
Fabian

> Cheers
> Kai Uwe
> 
> (Should probably do some REUSE porting as part of this, too ;)

Re: Fwd: Consistent CMake requirements in Plasma

[Fabian Vogt]

Moin,

Am Mittwoch, 24. Februar 2021, 14:40:08 CET schrieb Nicolas Fella:
> Hi packagers,
> 
> I recently proposed to formalize that Plasma depends on CMake 3.16. The
> reasoning is to 1) make the requirements consistent across Plasma repos
> and 2) enable the use of modern CMake features. If accpected this can
> act as a precedent for other non-frameworks KDE software.
> 
> Is there any distro that does not ship at least CMake 3.16 interested in
> shipping future Plasma releases?

Even openSUSE Leap 15.2 is at CMake 3.17 meanwhile, so that's fine for us.

Cheers,
Fabian

> Cheers
> 
> Nico
> 
>  Forwarded Message 
> Subject:  Consistent CMake requirements in Plasma
> Date: Tue, 9 Feb 2021 19:32:36 +0100
> From: Nicolas Fella 
> Reply-To: plasma-devel@kde.org
> To:   plasma-devel@kde.org
> 
> 
> 
> Hi,
> 
> while I was doing some cmake cleanup in Plasma I noticed that our stated
> minimum cmake versions are both somewhat inconsistent and super old.
> Most Plasma projects state that either 2.8 (released in 2009) or 3.0
> (released in 2014) are the minimum. That's obviously unrealistic.
> 
> Newer cmake versions offer some nice stuff such as imported targets for
> common libs (e.g. used in
> https://invent.kde.org/plasma/plasma-desktop/-/merge_requests/336). For
> the sake of consistency I propose that we standardize Plasma on
> something recent-ish.
> 
> My candidate for this would be cmake 3.16. It's the version shipped by
> Ubuntu 20.04 and thus Neon. This would exclude Debian stable which ships
> 3.13 (which does not have the feature relevant for
> https://invent.kde.org/plasma/plasma-desktop/-/merge_requests/336).
> 
> Thoughts?
> 
> Cheers
> 
> Nico
> 
> 

Re: Critical Denial of Service bugs in Discover

[Fabian Vogt]

Hi,

Am Samstag, 5. Februar 2022, 22:16:28 CET schrieb Ben Cooksley:
> Hi all,
> 
> Over the past week or so Sysadmin has been dealing with an extremely high
> volume of traffic directed towards both download.kde.org and
> distribute.kde.org.
> 
> This traffic volume is curious in so far that it is directed at two paths
> specifically:
> - distribute.kde.org/khotnewstuff/fonts-providers.xml
> - download.kde.org/ocs/providers.xml
> 
> The first path is an "internal only" host which we were redirecting a
> legacy path to prior to the resource being relocated to cdn.kde.org. The
> second path has been legacy for numerous years now (more than 5) and is
> replaced by autoconfig.kde.org.
> It is of extreme concern that these paths are still in use - especially the
> ocs/providers.xml one.
> 
>...
> 
> This indicates that the bug lies solely within Plasma's Discover component
> - more precisely it's updater.
> 
> Examining the origin of these requests has indicated that some clients are
> making requests to these paths well in excess of several times a minute
> with a number of IP addresses appearing more 60 times in a 1 minute sized
> sample window.

FWICT, this is caused by plasma-discover-update, which is triggered by the
DiscoverNotifier service if automatic updates are enabled in kcm_updates,
updates are available and the system idle for >=15min.

// If the system is untouched for 1 hour, trigger the unattened update
using namespace std::chrono_literals;
KIdleTime::instance()->addIdleTimeout(int(std::chrono::milliseconds(15min).count()));

(I wonder whether there's a bug about calling addIdleTimeout more than once.
It will then invoke triggerUpdate multiple times after 15min of idle.)

The Discover KNS backend creates instances for all available knsrc files,
which on construction call KNSReviews::setProviderUrl with the URL defined in
those files, triggering the requests.

The first URL is used by kfontinst.knsrc from plasma-workspace:
ProvidersUrl=https://distribute.kde.org/khotnewstuff/fonts-providers.xml

The second URL is used by multiple knsrc files in my VM:
aurorae.knsrc:ProvidersUrl=https://download.kde.org/ocs/providers.xml
comic.knsrc:ProvidersUrl=https://download.kde.org/ocs/providers.xml
kwineffect.knsrc:ProvidersUrl=https://download.kde.org/ocs/providers.xml
kwinscripts.knsrc:ProvidersUrl=https://download.kde.org/ocs/providers.xml
kwinswitcher.knsrc:ProvidersUrl=https://download.kde.org/ocs/providers.xml
wallpaperplugin.knsrc:ProvidersUrl=https://download.kde.org/ocs/providers.xml

> Given that Sysadmin has raised issues with this component and it's
> behaviour in the past, it appears that issues regarding the behaviour of
> the OCS componentry within Discover remain unresolved.
> 
> Due to the level of distress this is causing our systems, I am therefore
> left with no other option other than to direct the Plasma Discover
> developers to create and release without delay patches for all versions in
> support, as well as for all those currently present in any actively
> maintained distributions, that disable all OCS functionality in the
> Discover updater. Distributions are requested to treat these patches as
> security patches and to distribute them to users without delay.

Emergency workarounds for distributions might be to either not ship the KNS
backend by not building kns-backend.so or deleting it afterwards, or disabling
the discover notifier (/etc/xdg/autostart/org.kde.discover.notifier.desktop)
completely.

Cheers,
Fabian

Re: Critical Denial of Service bugs in Discover

[Fabian Vogt]

Moin,

Am Sonntag, 6. Februar 2022, 19:27:11 CET schrieb Ben Cooksley:
> On Sun, Feb 6, 2022 at 1:07 PM Fabian Vogt  wrote:
> > The first URL is used by kfontinst.knsrc from plasma-workspace:
> > ProvidersUrl=https://distribute.kde.org/khotnewstuff/fonts-providers.xml
> >
> > The second URL is used by multiple knsrc files in my VM:
> > aurorae.knsrc:ProvidersUrl=https://download.kde.org/ocs/providers.xml
> > comic.knsrc:ProvidersUrl=https://download.kde.org/ocs/providers.xml
> > kwineffect.knsrc:ProvidersUrl=https://download.kde.org/ocs/providers.xml
> > kwinscripts.knsrc:ProvidersUrl=https://download.kde.org/ocs/providers.xml
> > kwinswitcher.knsrc:ProvidersUrl=https://download.kde.org/ocs/providers.xml
> > wallpaperplugin.knsrc:ProvidersUrl=
> > https://download.kde.org/ocs/providers.xml
> 
> This makes me incredibly sad. We had a push to eliminate all usage of the
> legacy download.kde.org endpoint many years ago...
> I have now resolved the majority of these - if distributions could please
> pick up those patches that would be appreciated.
> 
> Please note that I have now terminated the support on the server that was
> making these legacy endpoints work, so those patches are necessary to
> restore functionality.

Does this decrease the server load noticably?

On IRC you wrote that the primary offender is
"KNewStuff/5.86.0-plasma-discover-update/", can you make the endpoints return
an error for the top entries only? Then we'll get bug reports only for the
likely cause(s) of the high traffic instead of everyone.

What might help is to have a lightweight proxy in front of Apache to handle
those paths in a way that doesn't stress the system much. That should probably
be investigated independently of the client side, as this is entirely under our
control and has immediate effects.

It's also possible that the requests aren't actually caused by Discover at all,
but just something which imitates it in a DDoS attack. In that case we couldn't
do anything on the client-side anyway. I don't think this is very likely, but
until the issue was reproduced with disover it's a possibility.

> > > Given that Sysadmin has raised issues with this component and it's
> > > behaviour in the past, it appears that issues regarding the behaviour of
> > > the OCS componentry within Discover remain unresolved.
> > >
> > > Due to the level of distress this is causing our systems, I am therefore
> > > left with no other option other than to direct the Plasma Discover
> > > developers to create and release without delay patches for all versions
> > in
> > > support, as well as for all those currently present in any actively
> > > maintained distributions, that disable all OCS functionality in the
> > > Discover updater. Distributions are requested to treat these patches as
> > > security patches and to distribute them to users without delay.
> >
> > Emergency workarounds for distributions might be to either not ship the KNS
> > backend by not building kns-backend.so or deleting it afterwards, or
> > disabling
> > the discover notifier
> > (/etc/xdg/autostart/org.kde.discover.notifier.desktop)
> > completely.
> 
> I have now committed patches to Discover going back to Plasma/5.18 which
> disable the build of the KNS backend.
> If distributions could please pick them up and distribute them as I
> previously indicated that would be much appreciated.
> 
> https://invent.kde.org/plasma/discover/-/commit/f66df3531670592960167f5060feeed6d6c792be

IMO we need a more targeted approach there. The main offenders aren't running
the latest version, so likely won't get those updates that quickly either.
If we have more data and can pinpoint it a bit better that would at least help
to speed patch delivery up.

> Please note that I intend to investigate whether it is possible to serve
> corrupted files from the server side that cause Discover to crash to help
> alleviate the load being created by those clients.

Sounds like a good way to DoS bugzilla instead and cause bad PR, both up- and
downstream. On top of that, it's possible that a resulting crashloop causes an
even higher frequency of requests.

Cheers,
Fabian

> Current load being generated by this is:
> 
> 789 requests/sec - 6.4 MB/second - 8.3 kB/request - 1.70113
> ms/request
> 217 requests currently being processed, 183 idle workers
> 
> > Cheers,
> > Fabian
> >
> Thanks,
> Ben

[plasma/plasma-workspace/Plasma/5.24] kcms/kfontinst/kcmfontinst: Use the correct endpoint for this - hosted by a CDN and therefore capable of handling much greater volumes of traffic.

[Fabian Vogt]

Git commit fb5656eaf2e021e6a9288edd00573c14afe6e115 by Fabian Vogt, on behalf 
of Ben Cooksley.
Committed on 07/02/2022 at 11:09.
Pushed by fvogt into branch 'Plasma/5.24'.

Use the correct endpoint for this - hosted by a CDN and therefore capable of 
handling much greater volumes of traffic.

CCMAIL: plasma-devel@kde.org
CCMAIL: distributi...@kde.org
(cherry picked from commit 8c11f207e2bbf0f98488de7066bd3698705f20e1)

M  +1-1kcms/kfontinst/kcmfontinst/kfontinst.knsrc

https://invent.kde.org/plasma/plasma-workspace/commit/fb5656eaf2e021e6a9288edd00573c14afe6e115

diff --git a/kcms/kfontinst/kcmfontinst/kfontinst.knsrc 
b/kcms/kfontinst/kcmfontinst/kfontinst.knsrc
index 99c195423..fd41b1f89 100644
--- a/kcms/kfontinst/kcmfontinst/kfontinst.knsrc
+++ b/kcms/kfontinst/kcmfontinst/kfontinst.knsrc
@@ -42,7 +42,7 @@ Name[vi]=Phông chữ
 Name[x-test]=xxFontsxx
 Name[zh_CN]=字体
 
-ProvidersUrl=https://distribute.kde.org/khotnewstuff/fonts-providers.xml
+ProvidersUrl=https://cdn.kde.org/khotnewstuff/v1/fonts-providers.xml
 Categories=KDE-Look.org Fonts
 TargetDir=kfontinst
 Uncompress=archive

Re: Critical Denial of Service bugs in Discover

[Fabian Vogt]

Moin,

Am Sonntag, 6. Februar 2022, 21:54:13 CET schrieb Fabian Vogt:
> Am Sonntag, 6. Februar 2022, 19:27:11 CET schrieb Ben Cooksley:
> > On Sun, Feb 6, 2022 at 1:07 PM Fabian Vogt  wrote:
> > > The first URL is used by kfontinst.knsrc from plasma-workspace:
> > > ProvidersUrl=https://distribute.kde.org/khotnewstuff/fonts-providers.xml
> > >
> > > The second URL is used by multiple knsrc files in my VM:
> > > aurorae.knsrc:ProvidersUrl=https://download.kde.org/ocs/providers.xml
> > > comic.knsrc:ProvidersUrl=https://download.kde.org/ocs/providers.xml
> > > kwineffect.knsrc:ProvidersUrl=https://download.kde.org/ocs/providers.xml
> > > kwinscripts.knsrc:ProvidersUrl=https://download.kde.org/ocs/providers.xml
> > > kwinswitcher.knsrc:ProvidersUrl=https://download.kde.org/ocs/providers.xml
> > > wallpaperplugin.knsrc:ProvidersUrl=
> > > https://download.kde.org/ocs/providers.xml
> > 
> > This makes me incredibly sad. We had a push to eliminate all usage of the
> > legacy download.kde.org endpoint many years ago...
> > I have now resolved the majority of these - if distributions could please
> > pick up those patches that would be appreciated.
> > 
> > Please note that I have now terminated the support on the server that was
> > making these legacy endpoints work, so those patches are necessary to
> > restore functionality.
> ...
> It's also possible that the requests aren't actually caused by Discover at 
> all,
> but just something which imitates it in a DDoS attack. In that case we 
> couldn't
> do anything on the client-side anyway. I don't think this is very likely, but
> until the issue was reproduced with disover it's a possibility.

I think I have a plausible explanation for what could've caused this.
While testing a MR for the notifier, I noticed odd behaviour: It always ran
plasma-discover-update twice!
https://invent.kde.org/plasma/discover/-/merge_requests/254#note_394584

The reason for that is that after the update process finishes, the notifier
realizes that it's idle again and if updates are available, it will immediately
trigger another update after the 15min idle time. Now here's the catch: If the
system has already been idle for >=15min (which is very likely at that point),
the idle timeout will immediately fire! This process repeats unlimited and
without delay, until the system is no longer idle or there aren't updates
available anymore. Here I have plasma-discover-update running approx. every
second, which amounts to ~4 req/s to download.kde.org.

This is mostly mitigated by the introduction of the 3h delay between updates
by d607e0c6f9, but not entirely. The check is only effective after the second
iteration, which is what I observed in my testing. (One of the commits in my MR
should address that as well.)

One of the conditions for running into this bug is that after the automatic
updater ran, there still have to be updates available to trigger the next run.
Initially I thought that this can mostly happen if updates fail to download or
install, this is unfortunately not true. The notifier by default counts all
available updates, but the updater only installs offline updates. So if there
is even a single non-offline update available, the loop continues.

So this probably affected a lot of users who enabled automatic installation of
updates :-/

Cheers,
Fabian

Re: Plasma 5.24.2

[Fabian Vogt]

Moin,

Am Dienstag, 22. Februar 2022, 14:56:11 CET schrieb Jonathan Riddell:
> Plasma 5.24.2 is now released https://kde.org/announcements/plasma/5/5.24.2

It appears like plasma-mobile-5.24.2.tar.xz is missing translations, like with
5.24.1 already.

Cheers,
Fabian

Re: Plasma 5.26.3: Breeze is broken

[Fabian Vogt]

Hi,

Am Dienstag, 8. November 2022, 15:28:54 CET schrieb Jonathan Riddell:
> Plasma 5.26.3 is now released https://kde.org/announcements/plasma/5/5.26.3

At least breeze has the wrong version in its CMakeLists.txt:

set(PROJECT_VERSION "5.26.2")

This means that plasma-workspace will silently ignore Breeze and instead use 
Plastik as default window decoration.

I did not check whether other repos/tarballs are affected.

Thanks,
Fabian

Re: Plasma 6.0.1

[Fabian Vogt]

Hi,

Am Mittwoch, 6. März 2024, 00:38:54 CET schrieb Jonathan Riddell:
> Plasma 6.0.1 is out now for packaging and distributing
> https://kde.org/announcements/plasma/6.0.1/

URL is broken, it's https://kde.org/announcements/plasma/6/6.0.1/.

Also, kpipewire fails to build because there's a syntax error in the version
bump. Heiko fixed that in
https://invent.kde.org/plasma/kpipewire/-/commit/df052bfa3c66d24109f40f18266ee057d1838b9b

How can that even happen? Version bumps should be scripted and ideally the CI
should run before tags are pushed.

Cheers,
Fabian

D27643: FileChooser: download remote files to a cache location to open them through the portal

[Fabian Vogt]

fvogt added a comment.


  In D27643#618125 , @jgrulich wrote:
  
  > I have never used fuse. I see you can use kio-fuse over dbus to mount a 
file, but you still have to unmount it, which will be a problem, because from 
the portal I don't know whether the app is still using it or not.
  
  
  `kio-fuse` was designed with this in mind and does not even support 
unmounting. When the file isn't being used anymore, it drops everything except 
what's needed to reopen the file when requested.
  
  > If anyone is familiar with fuse and have solution in mind, can you give me 
a hint?
  
  In this case it would be as easy as just calling `org.kde.KIOFuse 
org.kde.KIOFuse.VFS mountUrl` with the url and it gets a local path back.
  
  This should ideally be handled transparently by the KDE file dialog though.

REPOSITORY
  R838 Flatpak Support: KDE Portal for XDG Desktop

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

To: jgrulich, aacid, feverfew, fvogt
Cc: apol, ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, ahiemstra, mart

D27643: FileChooser: download remote files to a cache location to open them through the portal

[Fabian Vogt]

fvogt added a comment.


  In D27643#618310 , @jgrulich wrote:
  
  > In D27643#618132 , @fvogt wrote:
  >
  > > In D27643#618125 , @jgrulich 
wrote:
  > >
  > > > I have never used fuse. I see you can use kio-fuse over dbus to mount a 
file, but you still have to unmount it, which will be a problem, because from 
the portal I don't know whether the app is still using it or not.
  > >
  > >
  > > `kio-fuse` was designed with this in mind and does not even support 
unmounting. When the file isn't being used anymore, it drops everything except 
what's needed to reopen the file when requested.
  > >
  > > > If anyone is familiar with fuse and have solution in mind, can you give 
me a hint?
  > >
  > > In this case it would be as easy as just calling `org.kde.KIOFuse 
org.kde.KIOFuse.VFS mountUrl` with the url and it gets a local path back.
  > >
  > > This should ideally be handled transparently by the KDE file dialog 
though.
  >
  >
  > I will make the portal not to freeze the app when there is no local file 
selected and we can revisit this later. As I can see, there is not even a 
stable relase of kio-fuse,
  
  
  Correct.
  
  > it also requires the user to start it manually and pick a folder so the 
experience is not that seamless and as Fabian said, it would be better to have 
this handled transparently by the file dialog.
  
  It's an autolaunching DBus service. Just make the call and it's up and 
running.

REPOSITORY
  R838 Flatpak Support: KDE Portal for XDG Desktop

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

To: jgrulich, aacid, feverfew, fvogt
Cc: apol, ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, ahiemstra, mart

D27347: Only link to Qt5WebChannel if Qt5WebEngineWidgets available

[Fabian Vogt]

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


  I need some input on how to express `HAVE_QTWEBENGINEWIDGETS` with this. 
Currently it would fail to build if `Qt5WebEngineWidgets` is installed but 
`Qt5WebChannel` isn't.

REPOSITORY
  R111 KSysguard Library

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

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

D27347: Only link to Qt5WebChannel if Qt5WebEngineWidgets available

[Fabian Vogt]

fvogt added a comment.


  Currently `processui/scripting.cpp` has this:
  
#if HAVE_QTWEBENGINEWIDGETS
#include 
...
#endif
  
  So `Qt5WebEngineWidgets` without `Qt5WebChannel` will fail to build and 
`Qt5WebChannel` without `Qt5WebEngineWidgets` leads to scripting support 
getting disabled.
  My question is what the right way to handle this is. List both as optional 
dependencies and use `#if HAVE_QTWEBENGINEWIDGETS && HAVE_QTWEBCHANNEL` instead 
or raise a cmake error out in the former case?

REPOSITORY
  R111 KSysguard Library

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

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

D27347: Only link to Qt5WebChannel if Qt5WebEngineWidgets available

[Fabian Vogt]

fvogt updated this revision to Diff 76750.
fvogt added a comment.
This revision is now accepted and ready to land.


  Treat scripting as a feature instead

REPOSITORY
  R111 KSysguard Library

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27347?vs=75550&id=76750

BRANCH
  Plasma/5.18

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

AFFECTED FILES
  CMakeLists.txt
  config-ksysguard.h.cmake
  processui/CMakeLists.txt
  processui/scripting.cpp
  processui/scripting.h

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

D27347: Only link to Qt5WebChannel if Qt5WebEngineWidgets available

[Fabian Vogt]

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

INLINE COMMENTS

> lbeltrame wrote in CMakeLists.txt:86
> Should you add a message in case one of the two is not found?

In that case `WEBENGINE_SCRIPTING_ENABLED` is `FALSE` and it'll print that 
`Scripting plugin support` is not enabled.

At least in theory.

REPOSITORY
  R111 KSysguard Library

BRANCH
  Plasma/5.18

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

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

D27347: Only link to Qt5WebChannel if Qt5WebEngineWidgets available

[Fabian Vogt]

This revision was automatically updated to reflect the committed changes.
fvogt marked an inline comment as done.
Closed by commit R111:ba7f78716af6: Only link to Qt5WebChannel if 
Qt5WebEngineWidgets available (authored by fvogt).

REPOSITORY
  R111 KSysguard Library

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27347?vs=76750&id=76751

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

AFFECTED FILES
  CMakeLists.txt
  config-ksysguard.h.cmake
  processui/CMakeLists.txt
  processui/scripting.cpp
  processui/scripting.h

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

D28134: Add ColorUtils

[Fabian Vogt]

fvogt added inline comments.

INLINE COMMENTS

> colorutils.cpp:90
> +});
> +}
> +

If item is neither of those three, it would never call `pending->setValue` at 
all

> colorutils.cpp:103
> +
> +if (pending->value().isValid()) {
> + pending->setValue(luma(result->value().value()) > 0.5 ? 
> ColorUtils::Brightness::Light : ColorUtils::Brightness::Dark);

Is this supposed to be `result->value().isValid()` instead?

If no, how can `pending` be valid at this point`?

If yes, how can `result` be valid at this point? It would also mean that 
`ready` wouldn't get emitted again, which results in a leak.

REPOSITORY
  R169 Kirigami

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

To: cblack, #plasma, mart, davidedmundson
Cc: fvogt, davidedmundson, plasma-devel, fbampaloukas, GB_2, domson, 
dkardarakos, ngraham, apol, ahiemstra, mart

D28614: Add better player tab crash handling

[Fabian Vogt]

fvogt added a comment.


  If there is a false positive in the detection, how would those be handled? 
AFAICT the players would never appear in mpris again?

REPOSITORY
  R856 Plasma Browser Integration

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

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

D28614: Add better player tab crash handling

[Fabian Vogt]

fvogt added a comment.


  In D28614#642630 , @broulik wrote:
  
  > > If there is a false positive in the detection, how would those be 
handled? AFAICT the players would never appear in mpris again?
  >
  > I believe whenever a player starts playing again, it is propagated through 
MPRIS again. The `playerGone` handling is no different from the player being 
removed from DOM and being added back.
  
  
  Is there an "adding back" though? If I'm not completely wrong, for such a 
"ghost" tab, `currentPlayer()` would return `{}` and nothing happens anymore.

REPOSITORY
  R856 Plasma Browser Integration

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

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

D28614: Add better player tab crash handling

[Fabian Vogt]

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


  For false positives the player would get added again by the `playing` event, 
which is not ideal, but as it doesn't require a reload it's IMO close enough.

REPOSITORY
  R856 Plasma Browser Integration

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

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

D26185: Also disable automatic scaling on Qt >= 5.14

[Fabian Vogt]

fvogt added a comment.


  That sounds like a Qt bug, which fails to use the `QT_SCREEN_SCALE_FACTORS` 
value for pixmaps for some reason. Is it a multi-monitor setup? Can you open a 
new issue with details and a screenshot?

REPOSITORY
  R120 Plasma Workspace

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

To: ahartmetz, #plasma, apol, davidedmundson, fvogt
Cc: powersnail, acooligan, fvogt, asturmlechner, dfaure, davidedmundson, 
anthonyfieroni, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28705: Don't consider player gone when it only got temporarily added by us

[Fabian Vogt]

fvogt added a comment.


  So was the case handled by line 805 completely broken?

REPOSITORY
  R856 Plasma Browser Integration

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

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

D28705: Don't consider player gone when it only got temporarily added by us

[Fabian Vogt]

fvogt added a comment.


  In D28705#644982 , @broulik wrote:
  
  > It worked on Chrome, and the properties can be accessed from the same 
context.
  >  It's just that the mutation.removedNodes loop above which is in content 
script cannot access those JS properties, so I had to change it, so it can.
  
  
  Before this patch it didn't attempt to read the property on Chrome either 
though, so I don't see how the `player.parentNode.removeChild(player);` didn't 
immediately result in the mutation observer to call `playerGone`.
  
  The change looks good, but I'm wondering how it ever worked previously...

REPOSITORY
  R856 Plasma Browser Integration

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

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

D28706: Restore old Audio prototype after exportFunction

[Fabian Vogt]

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


  I can't find a better way to do this either...
  
  Some notes:
  
  - `window.eval` works as expected so if not entirely unlucky (old firefox?) 
this can be replaced by using `executeScript` completely (PR incoming)
  - On chrome it works by accident as `window.Audio.prototype` is `Object`. 
`exportFunction` seems to clean that up, presumably to prevent access to the 
content script's `Object` (which != `window.Object`...)
  
  So I'm wondering whether the other overrides should also set the protoype 
properly? Currently `window.Audio.prototype != Audio` on non-firefox.

REPOSITORY
  R856 Plasma Browser Integration

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

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

D28709: Implement executeScript for Firefox

[Fabian Vogt]

fvogt created this revision.
fvogt added reviewers: Plasma, broulik.
Herald added a project: Plasma.
fvogt requested review of this revision.

REVISION SUMMARY
  window.eval is exactly what we need, so we can drop the Firefox specific
  replacements like calls to exportFunction.

TEST PLAN
  Restarted firefox, loaded extension with changes.
  https://playcanv.as/p/44MRmJRU/ and google translate can be controlled over
  MPRIS.

REPOSITORY
  R856 Plasma Browser Integration

BRANCH
  ffeval

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

AFFECTED FILES
  extension/content-script.js

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

D28709: Implement executeScript for Firefox

[Fabian Vogt]

fvogt updated this revision to Diff 79729.
fvogt added a comment.


  Use full URL in comment

REPOSITORY
  R856 Plasma Browser Integration

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28709?vs=79728&id=79729

BRANCH
  ffeval

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

AFFECTED FILES
  extension/content-script.js

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

D28709: Implement executeScript for Firefox

[Fabian Vogt]

fvogt added a comment.


  Note: Remains to be tested on non-firefox and old firefox ESR.

REPOSITORY
  R856 Plasma Browser Integration

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

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

D28706: Restore old Audio prototype after exportFunction

[Fabian Vogt]

fvogt added a comment.


  D28709  conflicts/supersedes this now. 
`window.Audio.prototype != Audio`  is a separate issue though, which might 
still need fixing.

REPOSITORY
  R856 Plasma Browser Integration

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

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

D28709: Implement executeScript for Firefox

[Fabian Vogt]

fvogt added a comment.


  I'm not sure about the `// Firefox enforces Content-Security-Policy also for 
scripts injected by the content-script` part, but I'm not sure how to test it.

REPOSITORY
  R856 Plasma Browser Integration

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

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

D28709: Implement executeScript for Firefox

[Fabian Vogt]

fvogt added a comment.


  I just tried this with google translate on FF ESR 68(.1.0 IIRC) and it 
worked, but there was an error about the content security policy having blocked 
an eval. The error is gone if the extension is disabled.
  So this needs a test with spotify/nextcloud. Do you have any public URL?

REPOSITORY
  R856 Plasma Browser Integration

BRANCH
  ffeval

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

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

D28709: Implement executeScript for Firefox

[Fabian Vogt]

fvogt abandoned this revision.
fvogt added a comment.


  https://bugzilla.mozilla.org/show_bug.cgi?id=1591983 :-(
  
  I guess spotify had `unsave-eval`, but not `unsafe-inline`, so this method 
just breaks different pages...
  
  Apparently it's possible to add a `script` element with 
´src="moz-extension:..."`, but this would require moving all page scripts into 
a separate file.

REPOSITORY
  R856 Plasma Browser Integration

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

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

D28705: Don't consider player gone when it only got temporarily added by us

[Fabian Vogt]

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


  In D28705#645038 , @broulik wrote:
  
  > Yeah, wondering the same... maybe it didn't. Anyway this also fixes 
Spotify's previous/next buttons not working since when the player is gone we 
clear media session actions (not sure if that is someting we should do though)
  
  
  Some logging reveals that with Vivaldi on Google  translate there's a 
sequence of added, removed, added. After commenting out the 
`player.parentNode.removeChild(player);`, only an added event remains. So the 
removal triggers an add event?

REPOSITORY
  R856 Plasma Browser Integration

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

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

D28719: [Purpose Plugin] Detect cancelling the prompt more reliable

[Fabian Vogt]

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


  Looks fragile, but I don't have a better idea either.
  
  https://bugreports.qt.io/browse/QTBUG-56761 sigh

REPOSITORY
  R856 Plasma Browser Integration

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

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

D28709: Implement executeScript for Firefox

[Fabian Vogt]

fvogt added a comment.


  In D28709#645296 , @broulik wrote:
  
  > I've originally injected breeze scroll bar CSS as style with src in the 
extension but that also cause other issues where websites weren't allowed to 
access the different origin of the style sheet...
  
  
  Apparently moz-extension as URL doesn't count, so that should be possible to 
implement.
  
  In D28709#645320 , @broulik wrote:
  
  > So, back to my export function patch? :/
  
  
  I'll keep looking around, maybe I can find something. I doubt it though, as 
it seems to be an intentional barrier for security.

REPOSITORY
  R856 Plasma Browser Integration

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

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

D28791: Manually merge stored and default settings

[Fabian Vogt]

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


  Just have to make sure not to add anything to 
`DEFAULT_EXTENSION_SETTINGS.mpris.websiteSettings` now. Previously that 
would've been ignored.

INLINE COMMENTS

> utils.js:39
> +Object.keys(add).forEach((key) => {
> +if (!obj.hasOwnProperty(key)) {
> +obj[key] = add[key];

Why not if/else?

REPOSITORY
  R856 Plasma Browser Integration

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

To: broulik, #plasma, fvogt
Cc: 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

D28791: Manually merge stored and default settings

[Fabian Vogt]

fvogt accepted this revision.
fvogt added inline comments.

INLINE COMMENTS

> fvogt wrote in utils.js:39
> Why not if/else?

I'd invert the condition

REPOSITORY
  R856 Plasma Browser Integration

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

To: broulik, #plasma, fvogt
Cc: 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

D28893: Detect Vivaldi based on binary name

[Fabian Vogt]

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


  Tested, works. I didn't even notice that it broke...
  
  Will probably have to be refactored later to look at 
`environmentDescriptions` or `environmentNames` though.
  
  > Should be good for 5.18 - coreaddons should have been an implicit 
dependency anyway?
  
  Yup.
  
  > Its icon doesn't show up and I have no idea where the "vivadi" icon file it 
specifies in its desktop file is located
  
  Here it used `xdg-icon-resource install ...` to install the icon in an rpm 
%post script, so I've got `/usr/share/icons/hicolor/*x*/apps/vivaldi.png` as 
usual.

REPOSITORY
  R856 Plasma Browser Integration

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

To: broulik, #plasma, fvogt
Cc: 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

D28853: krunner: Prevent regression

[Fabian Vogt]

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


  Proper fix with refactoring will take too long, let's take this for now.

REPOSITORY
  R120 Plasma Workspace

BRANCH
  master

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

To: apol, #plasma, fvogt
Cc: fvogt, 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

D28853: krunner: Prevent regression

[Fabian Vogt]

This revision was automatically updated to reflect the committed changes.
Closed by commit R120:a28e110cbb15: krunner: Prevent regression (authored by 
apol, committed by fvogt).

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28853?vs=80192&id=80371

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

AFFECTED FILES
  krunner/CMakeLists.txt
  krunner/krunner.desktop.cmake

To: apol, #plasma, fvogt
Cc: fvogt, 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

D28939: Don't offer sending non-http(s) links and sources via KDE Connect

[Fabian Vogt]

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


  > Repository R120  
Plasma Workspace
  
  How did that happen?
  
  > Should we include "ftp"?
  
  I'd say yes, together with `ftps`
  
  > Ones that only do JS and have an empty target still get it, unfortunately.
  
  That sounds weird, none of the targets match...

REPOSITORY
  R120 Plasma Workspace

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

To: broulik, #plasma, fvogt
Cc: 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

D28658: [krdb] Drop GTK2 colour exporting

[Fabian Vogt]

fvogt added a comment.


  > FIXED-IN: 5.19.0 (or should we consider this a bugfix and land it on the 
stable branch?)
  
  At least in openSUSE we'll backport it to Plasma 5.18, but it might not be 
the kind of behaviour change desired in 5.18 at this point in time...

REPOSITORY
  R119 Plasma Desktop

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

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

D28936: Use a Proxy object to detect changes within the MediaMetadata

[Fabian Vogt]

fvogt added a comment.


  AFAICT (take with a grain of salt, I'm a JS n00b) this doesn't catch 
something like this:
  
let a = {};
navigator.mediaSession.metadata = a;
a["foo"] = "bar";
  
  And behaves weirdly if you do:
  
let a = navigator.mediaSession.metadata;
navigator.mediaSession.metadata = null;
a["foo"] = "bar";

INLINE COMMENTS

> content-script.js:716
> +if (this.metadata) {
> +// MediaMetadata is not a regular Object so we 
> cannot just JSON.stringify it
> +keys = 
> Object.getOwnPropertyNames(Object.getPrototypeOf(this.metadata));

That appears to work fine here

REPOSITORY
  R856 Plasma Browser Integration

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

To: broulik, #plasma, fvogt, ognarb, cblack
Cc: IlyaBizyaev, 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

D28936: Use a Proxy object to detect changes within the MediaMetadata

[Fabian Vogt]

fvogt added a comment.


  In D28936#656422 , @broulik wrote:
  
  > > this doesn't catch something like this
  >
  > Yeah it doesn't. I thought I could "monitor" an Object but the caller 
actually has to //use// the Proxy for it to detect anything :/
  >
  > Any ideas? :)
  
  
  Other than using an interval timer to sync regularly, not really. At least 
the first case seems impossible to fix.
  
  The second case could be fixed though by `navigator.mediaSession.metadata === 
this` or something like that...

REPOSITORY
  R856 Plasma Browser Integration

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

To: broulik, #plasma, fvogt, ognarb, cblack
Cc: IlyaBizyaev, 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

D28926: Signal player gone in "pagehide" not "beforeunload"

[Fabian Vogt]

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


  Didn't test myself, but apparently you did, so LGTM.

REPOSITORY
  R856 Plasma Browser Integration

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

To: broulik, #plasma, fvogt, ognarb
Cc: 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

D29533: [Reminder] Also advert Chromie store for Vivaldi

[Fabian Vogt]

fvogt requested changes to this revision.
fvogt added a comment.
This revision now requires changes to proceed.


  "Chromie"? :D
  
  For some reason I can't get the reminder to trigger.
  
  `dbus-monitor` shows
  
method call time=1588952594.025913 sender=:1.16 -> 
destination=org.kde.ActivityManager serial=1613 
path=/ActivityManager/Resources; interface=org.kde.ActivityManager.Resources; 
member=RegisterResourceEvent
   string "org.kde.libtaskmanager"
   uint32 0
   string "applications:vivaldi-stable.desktop"
   uint32 0
  
  But the `ResourceScoreUpdated` signal never fires.

REPOSITORY
  R856 Plasma Browser Integration

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

To: broulik, #plasma, fvogt
Cc: 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

D9875: Extend parsing ssh prompt

[Fabian Vogt]

fvogt added a comment.


  In D9875#675101 , @mkoller wrote:
  
  > This patch breaks usage for git (and probably others):
  >  git first asks for a "Username for 'https:" which leads to ksshaskpass 
open the input dialog but the typed-in user
  >  is no longer stored into the wallet!!
  >  (See case TypeClearText)
  >  This leads to git again and again ask for the Username on each invokation.
  >
  > Please ensure that even the Username is stored again _in the same folder_ 
in kwallet as before (e.g. below Passwords)
  >  otherwise it also breaks using existing passwords. E.g. reading
  >
  >   if (type != TypePassword) {
  >  QByteArray retrievedBytes;
  >  wallet->readEntry(identifier, retrievedBytes);
  >
  > is wrong (backwards incompatible).
  
  
  Can you make a patch?
  
  Having a quick look at the previous code, it seems like username and password 
shared the identifier, so they overwrote each other.

REPOSITORY
  R105 KDE SSH Password Dialog

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

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

D9875: Extend parsing ssh prompt

[Fabian Vogt]

fvogt added a comment.


  In D9875#675107 , @mkoller wrote:
  
  > They did not overwrite each other since the entry for the Username holds an 
URL _without_ user but the password entry did.
  
  
  That's pretty fragile, but would need to be kept for compatibility indeed.

REPOSITORY
  R105 KDE SSH Password Dialog

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

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

D9875: Extend parsing ssh prompt

[Fabian Vogt]

fvogt closed this revision.
fvogt added a comment.


  FTR: https://invent.kde.org/plasma/ksshaskpass/-/merge_requests/2

REPOSITORY
  R105 KDE SSH Password Dialog

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

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

D13776: Move the salt file existence check into the unprivileged process

[Fabian Vogt]

fvogt created this revision.
fvogt added reviewers: Plasma, aacid.
Restricted Application added a project: Plasma.
Restricted Application added a subscriber: plasma-devel.
fvogt requested review of this revision.

REVISION SUMMARY
  Otherwise the salt is always recreated on with ~ on NFS with root_squash.

TEST PLAN
  Not yet, will update once results back.

REPOSITORY
  R107 KWallet PAM Integration

BRANCH
  Plasma/5.12

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

AFFECTED FILES
  pam_kwallet.c

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

D13776: Move the salt file existence check into the unprivileged process

[Fabian Vogt]

fvogt updated this revision to Diff 36826.
fvogt added a comment.


  Rebase on master - this only makes sense with the user privileges salt file 
reading.

REPOSITORY
  R107 KWallet PAM Integration

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13776?vs=36815&id=36826

BRANCH
  master

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

AFFECTED FILES
  pam_kwallet.c

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

D13776: Move remaining salt file operations into unprivileged processes

[Fabian Vogt]

fvogt updated this revision to Diff 36827.
fvogt added a comment.


  Fix rebase error

REPOSITORY
  R107 KWallet PAM Integration

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13776?vs=36826&id=36827

BRANCH
  master

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

AFFECTED FILES
  pam_kwallet.c

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

D13776: Move remaining salt file operations into unprivileged processes

[Fabian Vogt]

fvogt added a comment.


  > Are you saying that there are files that root can't read but the user can?
  
  Exactly.

REPOSITORY
  R107 KWallet PAM Integration

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

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

D13776: Move remaining salt file operations into unprivileged processes

[Fabian Vogt]

fvogt added a comment.


  In D13776#289669 , @aacid wrote:
  
  > In D13776#289490 , @fvogt wrote:
  >
  > > > Are you saying that there are files that root can't read but the user 
can?
  > >
  > > Exactly.
  >
  >
  > This is really interesting and shows how little i know about stuff, can you 
point me to some info about when/how this happens?
  
  
  Search for nfs root squashing.

REPOSITORY
  R107 KWallet PAM Integration

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

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

D12405: [WIP] Per-screen scale factors on X11 using QT_SCREEN_SCALE_FACTORS

[Fabian Vogt]

fvogt added inline comments.

INLINE COMMENTS

> gladhorn wrote in outputconfig.cpp:145
> Why not use QScopedPointer here?

I just moved the code from kcm/src/widget.cpp.

REPOSITORY
  R104 KScreen

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

To: fvogt, #plasma
Cc: gladhorn, mart, hein, ngraham, graesslin, davidedmundson, plasma-devel, 
ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol

D14209: Listen for player "emptied" signal

[Fabian Vogt]

fvogt added a comment.


  If the extension translates "emptied" to "stopped", wouldn't it be compatible 
with older hosts?

REPOSITORY
  R856 Plasma Browser Integration

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

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

D14209: Listen for player "emptied" signal

[Fabian Vogt]

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


  Didn't test myself, but LGTM

REPOSITORY
  R856 Plasma Browser Integration

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

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

D13776: Move remaining salt file operations into unprivileged processes

[Fabian Vogt]

This revision was automatically updated to reflect the committed changes.
Closed by commit R107:c78e8ade9e2b: Move remaining salt file operations into 
unprivileged processes (authored by fvogt).

REPOSITORY
  R107 KWallet PAM Integration

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13776?vs=36827&id=38556

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

AFFECTED FILES
  pam_kwallet.c

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

D14599: Rename libbreezecommon to libbreezecommon5 and don't install .so links

[Fabian Vogt]

fvogt created this revision.
fvogt added reviewers: Breeze, zzag.
Restricted Application added a project: Plasma.
Restricted Application added a subscriber: plasma-devel.
fvogt requested review of this revision.

REVISION SUMMARY
  libbreezecommon is not meant to be used outside of breeze (it can't, no 
headers
  are installed) so the .so link is not necessary.
  
  Additionally, rename libbreezecommon.so.5 to libbreezecommon5.so.5 for 
symmetry
  with libbreezecommon4.so.5.

TEST PLAN
  Built and installed it - still works.

REPOSITORY
  R31 Breeze

BRANCH
  master

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

AFFECTED FILES
  kdecoration/CMakeLists.txt
  kstyle/CMakeLists.txt
  libbreezecommon/CMakeLists.txt

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

D14599: Rename libbreezecommon to libbreezecommon5 and don't install .so links

[Fabian Vogt]

fvogt updated this revision to Diff 39062.
fvogt added a comment.


  BUG: 397145

REPOSITORY
  R31 Breeze

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14599?vs=39061&id=39062

BRANCH
  master

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

AFFECTED FILES
  kdecoration/CMakeLists.txt
  kstyle/CMakeLists.txt
  libbreezecommon/CMakeLists.txt

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

D14599: Rename libbreezecommon to libbreezecommon5 and don't install .so links

[Fabian Vogt]

This revision was automatically updated to reflect the committed changes.
Closed by commit R31:763ec6d33541: Rename libbreezecommon to libbreezecommon5 
and don't install .so links (authored by fvogt).

REPOSITORY
  R31 Breeze

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14599?vs=39062&id=39064

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

AFFECTED FILES
  kdecoration/CMakeLists.txt
  kstyle/CMakeLists.txt
  libbreezecommon/CMakeLists.txt

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

D14822: Use QJSValue as method parameter type for the scripting interface

[Fabian Vogt]

fvogt created this revision.
fvogt added a reviewer: Plasma.
Herald added a project: Plasma.
fvogt requested review of this revision.

REVISION SUMMARY
  If a slot or Q_INVOKABLE has a QVariant as parameter and gets called
  from a QJSEngine's script, it receives a QJSValue wrapped as QVariant.
  To get a QVariant with the actual value wrapped, calling QJSValue::toVariant
  is necessary.
  
  I'm not entirely sure whether this is intentional behaviour of QJSEngine, but
  even if it's a bug we'll have to workaround it.
  
  BUG: 397338

TEST PLAN
  I have favorites in kickoff again.

REPOSITORY
  R120 Plasma Workspace

BRANCH
  master

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

AFFECTED FILES
  shell/scripting/applet.cpp
  shell/scripting/applet.h
  shell/scripting/configgroup.cpp
  shell/scripting/configgroup.h
  shell/scripting/scriptengine_v1.cpp

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

D14822: Use QJSValue as method parameter type for the scripting interface

[Fabian Vogt]

fvogt added inline comments.

INLINE COMMENTS

> anthonyfieroni wrote in applet.cpp:139
> So writeEntry gets QJSValue& now why you call again toVariant(), here 
> everywhere else.

KConfigGroup != ConfigGroup. d->configGroup is from kconfig and does not take 
QJSValue. That's the core of the bugfix.

REPOSITORY
  R120 Plasma Workspace

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

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

D14822: Use QJSValue as method parameter type for the scripting interface

[Fabian Vogt]

fvogt added a comment.


  A different approach of fixing this is to do something like
  
if(value.userType() == qMetaTypeId())
value = value.value().toVariant();
  
  in every function for every QVariant parameter. Advantages are that it's more 
obvious and the function's interface does not change.
  Disadvantages are that it has more overhead and more lines of code.

REPOSITORY
  R120 Plasma Workspace

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

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