D8849: [Folder View Filter Config] Use TableView for mime types

[Kai Uwe Broulik]

broulik added a comment.


  The default sorting is by name. I was just demonstrating in the screenshot 
that you could also sort by checked in order to quickly find the checked ones, 
sorry. There //are// hundreds of options.

REPOSITORY
  R119 Plasma Desktop

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

To: broulik, #plasma, #vdg, hein
Cc: abetts, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, sebas, apol, mart

D8441: Wallpaper: hide color or blur filling options for full filling mode

[Yunhe Guo]

This revision was automatically updated to reflect the committed changes.
Closed by commit R120:72450a2a49d2: Wallpaper: hide color or blur filling 
options for full filling mode (authored by guoyunhe).

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8441?vs=21241=22502

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

AFFECTED FILES
  wallpapers/image/imagepackage/contents/ui/config.qml

To: guoyunhe, ngraham, jensreuterberg
Cc: jensreuterberg, broulik, ngraham, davidedmundson, plasma-devel, ZrenBot, 
progwolff, lesliezhai, ali-mohamed, abetts, sebas, apol, mart

D8858: Fix testWaylandFullscreenShell.

[Martin Flöser]

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


  Could you please add a message that it is missing?

REPOSITORY
  R127 KWayland

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

To: cgiboudeaux, graesslin
Cc: plasma-devel, #frameworks, leezu, ZrenBot, alexeymin, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D8849: [Folder View Filter Config] Use TableView for mime types

[Andres Betts]

abetts added a comment.


  In https://phabricator.kde.org/D8849#168791, @hein wrote:
  
  > I like it, but should the default sort column be the name so that there's 
no awkward arrow in the header element for the checkboxes?
  >
  > I feel like QWidget views or rather QStandardItemDelegate avoid this by 
embedding the checkbox into the column for Qt::DisplayRole or something ... 
maybe I misremember though.
  
  
  The sorter for checkboxes makes no sense. It is not necessary unless you feel 
the need to sort by ckecboxes checked or cleared. I imagine it would make more 
sense when there are hundreds of options. But you also have a sorting field. So 
no need.

REPOSITORY
  R119 Plasma Desktop

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

To: broulik, #plasma, #vdg, hein
Cc: abetts, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, sebas, apol, mart

D8849: [Folder View Filter Config] Use TableView for mime types

[Eike Hein]

hein added a comment.


  I like it, but should the default sort column be the name so that there's no 
awkward arrow in the header element for the checkboxes?
  
  I feel like QWidget views or rather QStandardItemDelegate avoid this by 
embedding the checkbox into the column for Qt::DisplayRole or something ... 
maybe I misremember though.

REPOSITORY
  R119 Plasma Desktop

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

To: broulik, #plasma, #vdg, hein
Cc: plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D8770: Resolve the input method issue by always force focus in search field.

[Xuetian Weng]

This revision was automatically updated to reflect the committed changes.
Closed by commit R119:44d8a7500e36: Resolve the input method issue by always 
force focus in search field. (authored by xuetianweng).

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8770?vs=22212=22494

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

AFFECTED FILES
  applets/kickoff/package/contents/ui/FullRepresentation.qml
  applets/kickoff/package/contents/ui/Header.qml

To: xuetianweng, hein, mart
Cc: ngraham, ihipop, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D8858: Fix testWaylandFullscreenShell.

[Christophe Giboudeaux]

cgiboudeaux created this revision.
cgiboudeaux added a reviewer: graesslin.
Restricted Application added subscribers: Frameworks, plasma-devel.
Restricted Application added projects: Plasma on Wayland, Frameworks.

REVISION SUMMARY
  This test needs the weston executable. Skip the test if the program wasn't 
found.

REPOSITORY
  R127 KWayland

BRANCH
  master

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

AFFECTED FILES
  autotests/client/CMakeLists.txt

To: cgiboudeaux, graesslin
Cc: plasma-devel, #frameworks, leezu, ZrenBot, alexeymin, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D8006: Add edit button to desktop theme

[Aleix Pol Gonzalez]

apol accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R119 Plasma Desktop

BRANCH
  master

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

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

D8856: Add support for new IdleInhibition protocol

[Martin Flöser]

graesslin created this revision.
graesslin added reviewers: KWin, Plasma.
Restricted Application added a project: KWin.
Restricted Application added subscribers: kwin, plasma-devel.

REVISION SUMMARY
  A small helper class is added which manages inhibiting idle for the
  ShellClients. So far only very basic functionality is added. That is
  only the inhibition on the Surface is followed. It is not yet checked
  whether the ShellClient is visible at all. That needs some changes in
  ShellClient.
  
  BUG: 385956
  FIXED-IN: 5.12

TEST PLAN
  New test case passes

REPOSITORY
  R108 KWin

BRANCH
  idle-inhibit

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

AFFECTED FILES
  CMakeLists.txt
  autotests/integration/CMakeLists.txt
  autotests/integration/idle_inhibition_test.cpp
  autotests/integration/kwin_wayland_test.h
  autotests/integration/test_helpers.cpp
  idle_inhibition.cpp
  idle_inhibition.h
  wayland_server.cpp

To: graesslin, #kwin, #plasma
Cc: plasma-devel, kwin, bwowk, ZrenBot, progwolff, lesliezhai, ali-mohamed, 
hardening, jensreuterberg, abetts, sebas, apol, mart

D8661: Allow a cross-process check for same applications

[Martin Flöser]

This revision was automatically updated to reflect the committed changes.
Closed by commit R108:1ae7990a959c: Allow a cross-process check for same 
applications (authored by graesslin).

REPOSITORY
  R108 KWin

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8661?vs=21895=22483

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

AFFECTED FILES
  abstract_client.cpp
  abstract_client.h
  activation.cpp
  client.cpp
  client.h
  group.cpp
  shell_client.cpp
  shell_client.h
  tabbox/tabbox.cpp

To: graesslin, #kwin, #plasma, davidedmundson
Cc: plasma-devel, kwin, bwowk, ZrenBot, progwolff, lesliezhai, ali-mohamed, 
hardening, jensreuterberg, abetts, sebas, apol, mart

D8396: Add support for zwp_idle_inhibit_manager_v1

[Martin Flöser]

This revision was automatically updated to reflect the committed changes.
Closed by commit R127:9520c2f292d0: Add support for zwp_idle_inhibit_manager_v1 
(authored by graesslin).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D8396?vs=21046=22482#toc

REPOSITORY
  R127 KWayland

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8396?vs=21046=22482

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

AFFECTED FILES
  autotests/client/test_wayland_registry.cpp
  autotests/client/test_wayland_surface.cpp
  src/client/CMakeLists.txt
  src/client/idleinhibit.cpp
  src/client/idleinhibit.h
  src/client/protocols/idle-inhibit-unstable-v1.xml
  src/client/registry.cpp
  src/client/registry.h
  src/server/CMakeLists.txt
  src/server/display.cpp
  src/server/display.h
  src/server/idleinhibit_interface.cpp
  src/server/idleinhibit_interface.h
  src/server/idleinhibit_interface_p.h
  src/server/idleinhibit_interface_v1.cpp
  src/server/surface_interface.cpp
  src/server/surface_interface.h
  src/server/surface_interface_p.h
  src/tools/mapping.txt

To: graesslin, #frameworks, #kwin, #plasma_on_wayland, davidedmundson
Cc: davidedmundson, plasma-devel, leezu, ZrenBot, alexeymin, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D8854: Add enclosure registeration config for mycroft skills manager

[Aditya Mehra]

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

REVISION SUMMARY
  Added system wide mycroft configuration that registers the enclosure to be 
able to fetch enclosure specific skills via MSM

REPOSITORY
  R846 Mycroft Plasma integration

BRANCH
  master

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

AFFECTED FILES
  CMakeLists.txt
  PACKAGING.readme
  Readme.md
  mycroft/mycroft.conf

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

D8854: Add enclosure registeration config for mycroft skills manager

[Aditya Mehra]

This revision was automatically updated to reflect the committed changes.
Closed by commit R846:9b29a1bd83c7: Add enclosure registeration config for 
mycroft skills manager (authored by Aiix).

REPOSITORY
  R846 Mycroft Plasma integration

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8854?vs=22479=22480

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

AFFECTED FILES
  CMakeLists.txt
  PACKAGING.readme
  Readme.md
  mycroft/mycroft.conf

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

D8845: Create tooltipManager on demand

[Martin Flöser]

graesslin added a comment.


  In https://phabricator.kde.org/D8845#168632, @broulik wrote:
  
  > Look in the constructor, kept it consistent
  
  
  ah I just made this as a proposal as you had to change unrelated lines.

REPOSITORY
  R124 System Settings

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

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

D8845: Create tooltipManager on demand

[Kai Uwe Broulik]

broulik added a comment.


  Look in the constructor, kept it consistent

REPOSITORY
  R124 System Settings

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

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

D8845: Create tooltipManager on demand

[David Edmundson]

davidedmundson added a comment.


  I think you've identified something, (well, 0.2% on my PC) , which is good, 
but I think we should delve deeper into the issue.
  
  KTooltipManager isn't slow, it's the platform window being created in 
KTooltipWidget constructor that is.
  We can just move that code there.

REPOSITORY
  R124 System Settings

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

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

D8845: Create tooltipManager on demand

[Martin Flöser]

graesslin added inline comments.

INLINE COMMENTS

> SidebarMode.cpp:236
>  
>  ToolTipManager *toolTipManager;
>  QQuickWidget * quickWidget;

toolTipManager = nullptr;

REPOSITORY
  R124 System Settings

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

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

D8850: WIP: support drag and drop between shared folder view containments

[Kai Uwe Broulik]

broulik added a comment.


  I think only drops between //containments// should be special-cased.

REPOSITORY
  R119 Plasma Desktop

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

To: mwolff, amantia
Cc: broulik, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D8850: WIP: support drag and drop between shared folder view containments

[Milian Wolff]

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

REVISION SUMMARY
  This is still super buggy. From what I've understood so far, the
  problem seems to be the positioner acting up - it messes up its
  mapping, leading to some files showing up multiple times or not at
  all. The multiple-files issue is workarounded by ensuring the mapping
  stays unique, but I feel like it's the wrong stop-gap measure. Also
  I have no clue yet why the files sometimes disappear completely...

REPOSITORY
  R119 Plasma Desktop

BRANCH
  wip/lim-2

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

AFFECTED FILES
  containments/desktop/package/contents/ui/FolderView.qml
  containments/desktop/plugins/folder/foldermodel.cpp
  containments/desktop/plugins/folder/positioner.cpp
  containments/desktop/plugins/folder/screenmapper.cpp

To: mwolff, amantia
Cc: plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D8006: Add edit button to desktop theme

[David Edmundson]

davidedmundson marked 2 inline comments as done.

REPOSITORY
  R119 Plasma Desktop

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

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

D8849: [Folder View Filter Config] Use TableView for mime types

[Kai Uwe Broulik]

broulik created this revision.
broulik added reviewers: Plasma, VDG, hein.
Restricted Application added a project: Plasma.
Restricted Application added a subscriber: plasma-devel.

REVISION SUMMARY
  - Implements proper keyboard navigation (up/down) as well as space to goggle 
check box
  - Makes list sortable by name, comment, and "checked" (so you easily get an 
overview of types you enabled)
  - Add mime type "comment" which is more user-friendly

TEST PLAN
  - The checkbox becomes hardly visible on a selected row. In QWidgets Breeze 
has a hack to paint in white in this case but it cannot be done int QQC :/
  - Tested keyboard up and down as well as space to toggle
  - Sorting and filtering (the filter still only searches the name, not the 
comment) doesn't seem to confuse the table view or model
  
  F5494272: Screenshot_20171116_163712.png 


REPOSITORY
  R119 Plasma Desktop

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

AFFECTED FILES
  containments/desktop/package/contents/ui/ConfigFilter.qml
  containments/desktop/plugins/folder/mimetypesmodel.cpp

To: broulik, #plasma, #vdg, hein
Cc: plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D8641: FormLayout

[Marco Martin]

mart added inline comments.

INLINE COMMENTS

> broulik wrote in FormLayout.qml:45
> Use `Qt.callLater` (new in Qt 5.8) if you pass it an actual function (ie. not 
> an inline one) it will compress subsequent calls to one: 
> https://doc.qt.io/qt-5/qml-qtqml-qt.html#callLater-method
> 
> There's probably more places in the code where you could do this.

it's in frameworks, so i want to keep it still working as much as possible with 
5.7

> broulik wrote in formlayoutattached.cpp:28
> This class does not have an `eventFilter`

ah, leftover when the mnemonics stuff was implemented here

> broulik wrote in formlayoutattached.h:56
> Unused, there's also no property.

other leftovers, sorry :)

REPOSITORY
  R169 Kirigami

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

To: mart, #plasma, #kirigami, hein
Cc: broulik, colomar, ngraham, davidedmundson, plasma-devel, ZrenBot, 
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart, 
hein

D8692: QML mouse cursor KCM and components

[Marco Martin]

mart added a comment.


  In https://phabricator.kde.org/D8692#168581, @ngraham wrote:
  
  > A scrollbar to show hidden buttons is a pretty bad UX. We should aim for 
something better in the new version. Perhaps the buttons should lose their text 
and become icons-only when the window is too narrow to show them all with icons 
and text.
  
  
  or perhaps relayout to 2 lines...

REPOSITORY
  R119 Plasma Desktop

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

To: mart, #plasma
Cc: aspotashev, januz, ngraham, subdiff, plasma-devel, ZrenBot, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D8641: FormLayout

[Marco Martin]

mart updated this revision to Diff 22468.
mart marked 10 inline comments as done.
mart added a comment.


  - adress comments

REPOSITORY
  R169 Kirigami

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8641?vs=22462=22468

BRANCH
  mart/formlayout

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

AFFECTED FILES
  examples/galleryapp/resources.qrc
  examples/gallerydata/contents/ui/MainPage.qml
  examples/gallerydata/contents/ui/gallery/ButtonGallery.qml
  examples/gallerydata/contents/ui/gallery/FormLayoutGallery.qml
  examples/gallerydata/contents/ui/gallery/TextFieldGallery.qml
  kirigami.qrc
  src/CMakeLists.txt
  src/controls/FormLayout.qml
  src/controls/GlobalDrawer.qml
  src/controls/Heading.qml
  src/controls/private/PrivateActionToolButton.qml
  src/controls/templates/FormLayout.qml
  src/formlayoutattached.cpp
  src/formlayoutattached.h
  src/kirigamiplugin.cpp
  src/mnemonicattached.cpp
  src/mnemonicattached.h

To: mart, #plasma, #kirigami, hein
Cc: broulik, colomar, ngraham, davidedmundson, plasma-devel, ZrenBot, 
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart, 
hein

D8006: Add edit button to desktop theme

[David Edmundson]

davidedmundson updated this revision to Diff 22467.
davidedmundson added a comment.


  Use QProcess

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8006?vs=19973=22467

BRANCH
  master

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

AFFECTED FILES
  kcms/desktoptheme/kcm.cpp
  kcms/desktoptheme/kcm.h
  kcms/desktoptheme/package/contents/ui/main.qml

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

D8692: QML mouse cursor KCM and components

[Nathaniel Graham]

ngraham added a comment.


  A scrollbar to show hidden buttons is a pretty bad UX. We should aim for 
something better in the new version. Perhaps the buttons should lose their text 
and become icons-only when the window is too narrow to show them all with icons 
and text.

REPOSITORY
  R119 Plasma Desktop

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

To: mart, #plasma
Cc: aspotashev, januz, ngraham, subdiff, plasma-devel, ZrenBot, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D8809: Reduce spurious signal emissions

[Aleix Pol Gonzalez]

apol added inline comments.

INLINE COMMENTS

> davidedmundson wrote in framesvgitem.cpp:295
> If we always have 4 and can initialised them to 0 qreal[4] with a c style 
> array would be better.

Would it? it's harder to compare then as you can't do a simple `!=`

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  master

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

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

D8809: Reduce spurious signal emissions

[Aleix Pol Gonzalez]

This revision was automatically updated to reflect the committed changes.
Closed by commit R242:11c9206bf68f: Reduce spurious signal emissions (authored 
by apol).

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8809?vs=22304=22465

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

AFFECTED FILES
  src/declarativeimports/core/framesvgitem.cpp
  src/declarativeimports/core/framesvgitem.h
  src/plasma/framesvg.cpp

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

D8692: QML mouse cursor KCM and components

[Alexander Potashev]

aspotashev added a comment.


  >> Does it add a horizontal scrollbar when the width is not enough for all 
the pushbuttons, like in the QWidget-based KCM (screenshot below)?
  > 
  > this screenshot if from the old module, you have to actually unistall it or 
will keep loading that one
  
  I know. The question was about the new KCM. Some buttons may become hard to 
discover if there is no scroolbar anymore.

REPOSITORY
  R119 Plasma Desktop

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

To: mart, #plasma
Cc: aspotashev, januz, ngraham, subdiff, plasma-devel, ZrenBot, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D8522: keyboard navigation in and out QML kcms

[Marco Martin]

This revision was automatically updated to reflect the committed changes.
Closed by commit R295:9ebf898572ca: keyboard navigation in and out QML kcms 
(authored by mart).

REPOSITORY
  R295 KCMUtils

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8522?vs=22463=22464

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

AFFECTED FILES
  src/kcmoduleqml.cpp
  src/kcmoduleqml_p.h

To: mart, #plasma, #frameworks, davidedmundson
Cc: broulik, davidedmundson, ngraham, plasma-devel, #frameworks, ZrenBot, 
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D8522: keyboard navigation in and out QML kcms

[David Edmundson]

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

REPOSITORY
  R295 KCMUtils

BRANCH
  phab/keynav

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

To: mart, #plasma, #frameworks, davidedmundson
Cc: broulik, davidedmundson, ngraham, plasma-devel, #frameworks, ZrenBot, 
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D8692: QML mouse cursor KCM and components

[Marco Martin]

mart added a comment.


  In https://phabricator.kde.org/D8692#168408, @aspotashev wrote:
  
  > Does it add a horizontal scrollbar when the width is not enough for all the 
pushbuttons, like in the QWidget-based KCM (screenshot below)? F5494095: 
Screenshot_20171116_143922.png 
  
  
  this screenshot if from the old module, you have to actually unistall it or 
will keep loading that one

REPOSITORY
  R119 Plasma Desktop

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

To: mart, #plasma
Cc: aspotashev, januz, ngraham, subdiff, plasma-devel, ZrenBot, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D8522: keyboard navigation in and out QML kcms

[Marco Martin]

mart updated this revision to Diff 22463.
mart added a comment.


  - guard null

REPOSITORY
  R295 KCMUtils

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8522?vs=21440=22463

BRANCH
  phab/keynav

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

AFFECTED FILES
  src/kcmoduleqml.cpp
  src/kcmoduleqml_p.h

To: mart, #plasma, #frameworks, davidedmundson
Cc: broulik, davidedmundson, ngraham, plasma-devel, #frameworks, ZrenBot, 
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D8522: keyboard navigation in and out QML kcms

[Marco Martin]

mart marked 2 inline comments as done.

REPOSITORY
  R295 KCMUtils

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

To: mart, #plasma, #frameworks, davidedmundson
Cc: broulik, davidedmundson, ngraham, plasma-devel, #frameworks, ZrenBot, 
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D8809: Reduce spurious signal emissions

[David Edmundson]

davidedmundson accepted this revision.
davidedmundson added inline comments.
This revision is now accepted and ready to land.

INLINE COMMENTS

> framesvgitem.cpp:295
> +
> +const QVector m_oldMargins;
> +FrameSvgItemMargins *const m_margins;

If we always have 4 and can initialised them to 0 qreal[4] with a c style array 
would be better.

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  master

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

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

D8526: add a background in ScrollView

[Marco Martin]

mart added a comment.


  In https://phabricator.kde.org/D8526#168377, @davidedmundson wrote:
  
  > > so, may be worth to have in kirigami a component called like 
ItemScrollView intended for this?
  >
  > I think so.
  >  The component can be just one line of ScrollView{}, and the styles can do 
something like
  
  
  hmm, tough if is in Kirigami i can't really use the desktop style private 
plugin to paint it, or would be cross dependency?
  
  > if typeOf(control) == "ItemView"
  
  afaik typeof doesn't work there, all qml item will just return "Object", so 
the only way is to testing the presence of some properties and hope it goes 
well :/

REPOSITORY
  R858 Qt Quick Controls 2: Desktop Style

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

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

D8641: FormLayout

[Marco Martin]

mart updated this revision to Diff 22462.
mart added a comment.


  - if there is a rendercontrol, watch the target window

REPOSITORY
  R169 Kirigami

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8641?vs=22410=22462

BRANCH
  mart/formlayout

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

AFFECTED FILES
  examples/galleryapp/resources.qrc
  examples/gallerydata/contents/ui/MainPage.qml
  examples/gallerydata/contents/ui/gallery/ButtonGallery.qml
  examples/gallerydata/contents/ui/gallery/FormLayoutGallery.qml
  examples/gallerydata/contents/ui/gallery/TextFieldGallery.qml
  kirigami.qrc
  src/CMakeLists.txt
  src/controls/FormLayout.qml
  src/controls/GlobalDrawer.qml
  src/controls/Heading.qml
  src/controls/private/PrivateActionToolButton.qml
  src/controls/templates/FormLayout.qml
  src/formlayoutattached.cpp
  src/formlayoutattached.h
  src/kirigamiplugin.cpp
  src/mnemonicattached.cpp
  src/mnemonicattached.h

To: mart, #plasma, #kirigami, hein
Cc: broulik, colomar, ngraham, davidedmundson, plasma-devel, ZrenBot, 
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart, 
hein

D8845: Create tooltipManager on demand

[Kai Uwe Broulik]

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

REVISION SUMMARY
  It takes a couple of milliseconds to be created and isn't vital for System 
Settings startup.

TEST PLAN
  Tooltips still work

REPOSITORY
  R124 System Settings

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

AFFECTED FILES
  sidebar/SidebarMode.cpp

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

D8598: FolderView: position files at drop event target position

[Milian Wolff]

mwolff updated this revision to Diff 22458.
mwolff edited the summary of this revision.
mwolff added a comment.


  update message

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8598?vs=22457=22458

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

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

To: mwolff, hein, amantia
Cc: plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D8598: FolderView: position files at drop event target position

[Milian Wolff]

mwolff updated this revision to Diff 22457.
mwolff added a comment.


  use filename in mapping to support desktop:/

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8598?vs=21775=22457

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

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

To: mwolff, hein, amantia
Cc: plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D8834: Make it possible to have videos on the background of breeze

[Aleix Pol Gonzalez]

apol abandoned this revision.
apol added a comment.


  Here's the fork as requested `kde:scratch/apol/breeze-video-sddm`.
  
  TBH it feels a shame to fork 1200 lines of code for rejecting a patch that 
basically added 1. ¯\_(ツ)_/¯

REPOSITORY
  R120 Plasma Workspace

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

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

D8807: Fix warning regarding blur setting

[Aleix Pol Gonzalez]

This revision was automatically updated to reflect the committed changes.
Closed by commit R120:ddaf6f5696bd: Fix warning regarding blur setting 
(authored by apol).

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8807?vs=22302=22456

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

AFFECTED FILES
  wallpapers/image/imagepackage/contents/config/main.xml

To: apol, #plasma, guoyunhe, ngraham, broulik
Cc: plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D8493: Make Folder View screen aware

[Milian Wolff]

mwolff added a comment.


[14:24]  andris: milian umm, how do I actually move files between 
screens?
[14:26]  the fact that you can actually resize it with 
Alt+right-click is a bug
[14:27]  kbroulik: dnd, which is WIP
[14:28]  milian: okay because right now the screen-aware FV does 
not work at all. other than that I dont have any icons on my second screen 
anymore :)
[14:28]  you mean the "position files at drop event"?
[14:28]  yes, but that is only one step
[14:29]  I'm working on the second, more important one
[14:29]  can you tell me what you mean by no icons on the second 
screen?
[14:29]  did you have some on the second screen and by applying the 
patch they got moved to the first screen?
[14:29]  i.e. some kind of config porting?
[14:29]  yes
[14:29]  andris: ^ that is useful important feedback, we need to 
handle that somehow
[14:29]  but kbroulik - how did you have icons on your second 
screen before?
[14:29]  milian: I have a FV on both screens, both pointing to 
desktop:/
[14:29]  did you use the same folder?
[14:30]  yes
[14:30]  then both used to show the same icons, no?
[14:30]  they don't
[14:30]  err
[14:30]  yes, they *used to*
[14:30]  so this is somewhat to be expected I guess
[14:30]  right so nothing to be ported after all
[14:30]  yans: well, that's how it is
[14:30]  So i just asking, kbroulik thx for info.
[14:30]  what's missing is the DND to allow people to move files 
actually
[14:30]  I'm working on that, keep getting sidetracked by other 
projects
[14:30]  let me sit down on it again
[14:35]  milian: ah, okay, because right now DND is unchanged, 
ie. I drag files over and it says "would overwrite" :)
[14:36]  yes that is broken as before
[14:36]  I'm working on that and have a local WIP for that though
[14:36]  oki :)
[14:36]  so I cannot really comment on D8493 let's have Sho_ 
decide
  
  so I guess we may need to keep this one open until the two follow-up DND 
patches are ready to allow this to be tested in full-depth

REPOSITORY
  R119 Plasma Desktop

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

To: amantia, #plasma, ervin, mlaurent, dvratil, hein, aacid, davidedmundson, 
apol, mwolff
Cc: anthonyfieroni, ngraham, mwolff, davidedmundson, broulik, mart, 
plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol

D8493: Make Folder View screen aware

[David Edmundson]

davidedmundson added a comment.


  Fine with me.
  
  Would be good if Eike could comment

REPOSITORY
  R119 Plasma Desktop

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

To: amantia, #plasma, ervin, mlaurent, dvratil, hein, aacid, davidedmundson, 
apol, mwolff
Cc: anthonyfieroni, ngraham, mwolff, davidedmundson, broulik, mart, 
plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol

D8807: Fix warning regarding blur setting

[Kai Uwe Broulik]

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

REPOSITORY
  R120 Plasma Workspace

BRANCH
  master

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

To: apol, #plasma, guoyunhe, ngraham, broulik
Cc: plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D8807: Fix warning regarding blur setting

[Aleix Pol Gonzalez]

apol added a comment.


  Ping?

REPOSITORY
  R120 Plasma Workspace

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

To: apol, #plasma, guoyunhe, ngraham
Cc: plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D8493: Make Folder View screen aware

[Milian Wolff]

mwolff accepted this revision.
mwolff added a comment.


  lgtm from my side. Anyone from the plasma team care to chime in?

REPOSITORY
  R119 Plasma Desktop

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

To: amantia, #plasma, ervin, mlaurent, dvratil, hein, aacid, davidedmundson, 
apol, mwolff
Cc: anthonyfieroni, ngraham, mwolff, davidedmundson, broulik, mart, 
plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol

D8493: Make Folder View screen aware

[Andras Mantia]

amantia added inline comments.

INLINE COMMENTS

> mwolff wrote in screenmapper.cpp:85
> this should imo be a for loop to make it clear that it is iterating over all 
> items (more ideomatic). Also, couldn't you use range-based for here even?

range based loop: I need access to both key and value

> mwolff wrote in screenmapper.cpp:91
> this is actually shared with above, so that could be in a lambda

I don't see what can be extracted

REPOSITORY
  R119 Plasma Desktop

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

To: amantia, #plasma, ervin, mlaurent, dvratil, hein, aacid, davidedmundson, 
apol, mwolff
Cc: anthonyfieroni, ngraham, mwolff, davidedmundson, broulik, mart, 
plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol

D8493: Make Folder View screen aware

[Andras Mantia]

amantia updated this revision to Diff 22444.
amantia added a comment.


  Some code reogranization per Milian's request

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8493?vs=22438=22444

BRANCH
  master

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

AFFECTED FILES
  containments/desktop/package/contents/config/main.xml
  containments/desktop/package/contents/ui/FolderView.qml
  containments/desktop/package/contents/ui/FolderViewLayer.qml
  containments/desktop/plugins/folder/CMakeLists.txt
  containments/desktop/plugins/folder/autotests/CMakeLists.txt
  containments/desktop/plugins/folder/autotests/foldermodeltest.cpp
  containments/desktop/plugins/folder/autotests/foldermodeltest.h
  containments/desktop/plugins/folder/autotests/screenmappertest.cpp
  containments/desktop/plugins/folder/autotests/screenmappertest.h
  containments/desktop/plugins/folder/foldermodel.cpp
  containments/desktop/plugins/folder/foldermodel.h
  containments/desktop/plugins/folder/folderplugin.cpp
  containments/desktop/plugins/folder/screenmapper.cpp
  containments/desktop/plugins/folder/screenmapper.h

To: amantia, #plasma, ervin, mlaurent, dvratil, hein, aacid, davidedmundson, 
apol, mwolff
Cc: anthonyfieroni, ngraham, mwolff, davidedmundson, broulik, mart, 
plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol

D8493: Make Folder View screen aware

[Milian Wolff]

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


  some more nits, sorry for that ;-)

INLINE COMMENTS

> screenmapper.cpp:69
> +auto adjustFirstScreen = [this, ](const QString ) {
> +int firstScreen = m_firstScreenForPath.value(path, -1);
> +if (firstScreen == screenId) {

this could now be inlined below since it's only being used in one of the 
branches

> screenmapper.cpp:84
> +// needs to be updated.
> +const auto newFirstScreen = 
> std::min_element(m_availableScreens.constBegin(), 
> m_availableScreens.constEnd());
> +auto it = m_firstScreenForPath.begin();

this could be moved outside the branch and reused above, once the lambda is 
inlined

> screenmapper.cpp:85
> +const auto newFirstScreen = 
> std::min_element(m_availableScreens.constBegin(), 
> m_availableScreens.constEnd());
> +auto it = m_firstScreenForPath.begin();
> +while (it != m_firstScreenForPath.end()) {

this should imo be a for loop to make it clear that it is iterating over all 
items (more ideomatic). Also, couldn't you use range-based for here even?

> screenmapper.cpp:91
> +// we have now the path for the screen that was removed, so 
> adjust it
> +pathIt = m_screensPerPath.find(it.key());
> +if (pathIt != m_screensPerPath.end()) {

this is actually shared with above, so that could be in a lambda

REPOSITORY
  R119 Plasma Desktop

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

To: amantia, #plasma, ervin, mlaurent, dvratil, hein, aacid, davidedmundson, 
apol, mwolff
Cc: anthonyfieroni, ngraham, mwolff, davidedmundson, broulik, mart, 
plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol

D8840: XRandR: Clear EDID data when monitor is disconnected from an output

[David Edmundson]

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

REPOSITORY
  R110 KScreen Library

BRANCH
  Plasma/5.11

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

To: dvratil, sebas, davidedmundson
Cc: plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D8833: Make it possible to pass different kind of backgrounds as such

[Aleix Pol Gonzalez]

apol added a comment.


  > No for the same reason.
  
  How will one create a theme that supports video if they then can't put videos 
in it?

INLINE COMMENTS

> davidedmundson wrote in themeconfig.cpp:90
> So if it's blank (like the current Plasma theme with the blue image) a user 
> can never set one?

If there's just the color, `mBackgroundPath.isEmpty()` will be true.

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

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

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

D8840: XRandR: Clear EDID data when monitor is disconnected from an output

[Daniel Vrátil]

dvratil created this revision.
dvratil added a reviewer: sebas.
Restricted Application added a project: Plasma.

REVISION SUMMARY
  This fixes KScreen behavior in situation when a monitor is unplugged
  from an output and another monitor is plugged into the same output
  afterwards. By keeping the old monitor's EDID around, the KDED module
  would then mis-identify the newly connected monitor as the old one and
  would apply the wrong config.
  
  By clearing the EDID we force the backend to get an up-to-date EDID 
  from the new monitor, once ConfigMonitor requests it.

REPOSITORY
  R110 KScreen Library

BRANCH
  Plasma/5.11

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

AFFECTED FILES
  backends/xrandr/xrandroutput.cpp

To: dvratil, sebas
Cc: plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D8692: QML mouse cursor KCM and components

[Alexander Potashev]

aspotashev added a comment.


  Does it add a horizontal scrollbar when the width is not enough for all the 
pushbuttons, like in the QWidget-based KCM (screenshot below)? F5494095: 
Screenshot_20171116_143922.png 

REPOSITORY
  R119 Plasma Desktop

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

To: mart, #plasma
Cc: aspotashev, januz, ngraham, subdiff, plasma-devel, ZrenBot, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D8493: Make Folder View screen aware

[Andras Mantia]

amantia updated this revision to Diff 22438.
amantia added a comment.


  Handle screen removal correctly:
  
  - adjust the first screen per path in a more efficient way
  - adjust the number of screens per path properly

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8493?vs=22434=22438

BRANCH
  master

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

AFFECTED FILES
  containments/desktop/package/contents/config/main.xml
  containments/desktop/package/contents/ui/FolderView.qml
  containments/desktop/package/contents/ui/FolderViewLayer.qml
  containments/desktop/plugins/folder/CMakeLists.txt
  containments/desktop/plugins/folder/autotests/CMakeLists.txt
  containments/desktop/plugins/folder/autotests/foldermodeltest.cpp
  containments/desktop/plugins/folder/autotests/foldermodeltest.h
  containments/desktop/plugins/folder/autotests/screenmappertest.cpp
  containments/desktop/plugins/folder/autotests/screenmappertest.h
  containments/desktop/plugins/folder/foldermodel.cpp
  containments/desktop/plugins/folder/foldermodel.h
  containments/desktop/plugins/folder/folderplugin.cpp
  containments/desktop/plugins/folder/screenmapper.cpp
  containments/desktop/plugins/folder/screenmapper.h

To: amantia, #plasma, ervin, mlaurent, dvratil, hein, aacid, davidedmundson, 
apol, mwolff
Cc: anthonyfieroni, ngraham, mwolff, davidedmundson, broulik, mart, 
plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol

D8522: keyboard navigation in and out QML kcms

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> davidedmundson wrote in kcmoduleqml.cpp:154
> can this be null?

Quick glance at `QQuickItemPrivate::nextPrevItemInTabFocusChain` shows it 
cannot. In doubt it returns `this`.

REPOSITORY
  R295 KCMUtils

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

To: mart, #plasma, #frameworks, davidedmundson
Cc: broulik, davidedmundson, ngraham, plasma-devel, #frameworks, ZrenBot, 
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D8522: keyboard navigation in and out QML kcms

[David Edmundson]

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

INLINE COMMENTS

> kcmoduleqml.cpp:143
> +{
> +//FIXME: those are all workarounds around the QQuickWidget brokeness
> +if (watched == d->quickWidget && event->type() == QEvent::KeyPress) {

This needs a link to an upstream bug report.

> kcmoduleqml.cpp:148
> +
> +QQuickItem *currentItem = d->quickWindow->activeFocusItem();
> +if (currentItem->scopedFocusItem()) {

this can be null

> kcmoduleqml.cpp:154
> +if (ke->key() == Qt::Key_Tab) {
> +QQuickItem *nextItem = currentItem->nextItemInFocusChain(true);
> +//when it arrives at the place holder item, go out of the qqw and

can this be null?

REPOSITORY
  R295 KCMUtils

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

To: mart, #plasma, #frameworks, davidedmundson
Cc: davidedmundson, ngraham, plasma-devel, #frameworks, ZrenBot, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D8526: add a background in ScrollView

[David Edmundson]

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


  > so, may be worth to have in kirigami a component called like ItemScrollView 
intended for this?
  
  I think so.
  The component can be just one line of ScrollView{}, and the styles can do 
something like
  
  if typeOf(control) == "ItemView"
  
  Also we shoudn't use "edit" as the elementType for something that's not a 
textedit now that we control the C++ classes.

REPOSITORY
  R858 Qt Quick Controls 2: Desktop Style

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

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

D8493: Make Folder View screen aware

[Milian Wolff]

mwolff added inline comments.

INLINE COMMENTS

> screenmapper.cpp:82
> +// the screen got completely removed, not only its path changed
> +pathIt = m_screensPerPath.begin();
> +while (pathIt != m_screensPerPath.end()) {

personally I'd make this code explicit. i.e. this here is somewhat harder to 
grasp I think (and also slower, due to the repeated lookups) to something like 
this:

  const auto newFirstScreen = std::min_element(m_availableScreens.constBegin(), 
m_availableScreens.constEnd());
  for (auto  : m_firstScreenForPath) {
  if (screen == screenId) {
  screen = newFirstScreen;
  }
  }

potentially we also need to update m_screensPerPath though, which the current 
code doesn't do either. The branch above does write to `pathIt` though, is this 
missing here?

> screenmapper.cpp:105
> +for (const auto : it.value()) {
> +const auto url = QUrl(name);
> +// add the items to the new screen, if they are on a disabled 
> screen and their

unused variable, remove

REPOSITORY
  R119 Plasma Desktop

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

To: amantia, #plasma, ervin, mlaurent, dvratil, hein, aacid, davidedmundson, 
apol, mwolff
Cc: anthonyfieroni, ngraham, mwolff, davidedmundson, broulik, mart, 
plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol

D8493: Make Folder View screen aware

[Andras Mantia]

amantia updated this revision to Diff 22434.
amantia added a comment.


  Remove unused var

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8493?vs=22428=22434

BRANCH
  master

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

AFFECTED FILES
  containments/desktop/package/contents/config/main.xml
  containments/desktop/package/contents/ui/FolderView.qml
  containments/desktop/package/contents/ui/FolderViewLayer.qml
  containments/desktop/plugins/folder/CMakeLists.txt
  containments/desktop/plugins/folder/autotests/CMakeLists.txt
  containments/desktop/plugins/folder/autotests/foldermodeltest.cpp
  containments/desktop/plugins/folder/autotests/foldermodeltest.h
  containments/desktop/plugins/folder/autotests/screenmappertest.cpp
  containments/desktop/plugins/folder/autotests/screenmappertest.h
  containments/desktop/plugins/folder/foldermodel.cpp
  containments/desktop/plugins/folder/foldermodel.h
  containments/desktop/plugins/folder/folderplugin.cpp
  containments/desktop/plugins/folder/screenmapper.cpp
  containments/desktop/plugins/folder/screenmapper.h

To: amantia, #plasma, ervin, mlaurent, dvratil, hein, aacid, davidedmundson, 
apol, mwolff
Cc: anthonyfieroni, ngraham, mwolff, davidedmundson, broulik, mart, 
plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol

D8641: FormLayout

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> FormLayoutGallery.qml:79
> +text: item ? "Remove Field" : "Add Field"
> +property var item
> +onClicked: {

`property Item`?

That's in a couple of places

> FormLayout.qml:45
> +
> +Timer {
> +id: hintCompression

Use `Qt.callLater` (new in Qt 5.8) if you pass it an actual function (ie. not 
an inline one) it will compress subsequent calls to one: 
https://doc.qt.io/qt-5/qml-qtqml-qt.html#callLater-method

There's probably more places in the code where you could do this.

> formlayoutattached.cpp:28
> +m_buddyFor = qobject_cast(parent);
> +qApp->installEventFilter(this);
> +}

This class does not have an `eventFilter`

> formlayoutattached.h:37
> +
> +explicit FormLayoutAttached(QObject *parent = 0);
> +~FormLayoutAttached();

`nullptr`

> formlayoutattached.h:38
> +explicit FormLayoutAttached(QObject *parent = 0);
> +~FormLayoutAttached();
> +

`override`

> formlayoutattached.h:56
> +void isSectionChanged();
> +void mnemonicChanged();
> +void decoratedLabelChanged();

Unused, there's also no property.

> formlayoutattached.h:57
> +void mnemonicChanged();
> +void decoratedLabelChanged();
> +

Also unused

> formlayoutattached.h:61
> +QString m_label;
> +QString m_actualDecoratedLabel;
> +QString m_decoratedLabel;

Unused

> formlayoutattached.h:62
> +QString m_actualDecoratedLabel;
> +QString m_decoratedLabel;
> +QPointer  m_buddyFor;

Unused

> mnemonicattached.cpp:52
> +{
> +if (s_objectToSequence.contains(this)) {
> +s_sequenceToObject.remove(s_objectToSequence.value(this));

Double lookup (contains+value), also below

> mnemonicattached.cpp:62
> +
> +if (m_richTextLabel.length() == 0) {
> +return false;

`isEmpty`

> mnemonicattached.cpp:167
> +m_mnemonicLabel = m_actualRichTextLabel;
> +emit mnemonicLabelChanged();
> +emit richTextLabelChanged();

Can we emit this stuff only if actually changed or can this be assumed here?

> mnemonicattached.cpp:229
> +{
> +return m_actualRichTextLabel.length() > 0 ? m_actualRichTextLabel : 
> m_label;
> +}

`!isEmpty?

> mnemonicattached.cpp:287
> +} else {
> +m_weight = m_baseWeight + m_weights.keys().last();
> +}

`(m_weights.constEnd() - 1).key()`? Avoids creating a `keys()` temporary list

> mnemonicattached.h:38
> +Q_PROPERTY(QKeySequence sequence READ sequence NOTIFY sequenceChanged)
> +Q_ENUMS(ControlType)
> +public:

Use `Q_ENUM`

> mnemonicattached.h:48
> +
> +explicit MnemonicAttached(QObject *parent = 0);
> +~MnemonicAttached();

`nullptr`

REPOSITORY
  R169 Kirigami

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

To: mart, #plasma, #kirigami, hein
Cc: broulik, colomar, ngraham, davidedmundson, plasma-devel, ZrenBot, 
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart, 
hein

D8522: keyboard navigation in and out QML kcms

[Marco Martin]

mart added a comment.


  ping

REPOSITORY
  R295 KCMUtils

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

To: mart, #plasma, #frameworks
Cc: ngraham, plasma-devel, #frameworks, ZrenBot, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D8692: QML mouse cursor KCM and components

[Marco Martin]

mart added a comment.


  ping

REPOSITORY
  R119 Plasma Desktop

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

To: mart, #plasma
Cc: januz, ngraham, subdiff, plasma-devel, ZrenBot, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D8526: add a background in ScrollView

[Marco Martin]

mart added a comment.


  ping

REPOSITORY
  R858 Qt Quick Controls 2: Desktop Style

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

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

D8493: Make Folder View screen aware

[Andras Mantia]

amantia added inline comments.

INLINE COMMENTS

> mwolff wrote in screenmapper.cpp:108
> `m_itemsOnDisabledScreensMap` stores "names" from the `m_screenItemMap`, so I 
> think this line here is wrong and should also use the proposed 
> `screenUrlForPath` above, or `QUrl::fromUserInput`, no? Otherwise you may end 
> up with relative urls for file paths?

I don't understand the problem. Yes m_itemsOnDisabledScreensMap is storing the 
names (well, paths) from m_screenItemMap. They will be in a format like 
file://foo or desktop://foo or similar. screenPathWithScheme is the base path 
for the screen which is either file://SOMEPATH (or some other scheme) or 
desktop:/ . The check is to see if the items from the disabled screen map are 
under this path or not. I don't see where we can get relative paths. I might 
miss something though, but I don't know what. :)

> mwolff wrote in screenmapper.cpp:140
> is `name` also a `path`?

Basically yes, it is a path with scheme, but I renamed from "url" to "name" to 
be clear this is not a QUrl. I can change to path, although that is not exactly 
correct either, as the scheme is included.

REPOSITORY
  R119 Plasma Desktop

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

To: amantia, #plasma, ervin, mlaurent, dvratil, hein, aacid, davidedmundson, 
apol, mwolff
Cc: anthonyfieroni, ngraham, mwolff, davidedmundson, broulik, mart, 
plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol

D8493: Make Folder View screen aware

[Andras Mantia]

amantia updated this revision to Diff 22428.
amantia marked 2 inline comments as done.
amantia added a comment.


  Add a code comment

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8493?vs=22387=22428

BRANCH
  master

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

AFFECTED FILES
  containments/desktop/package/contents/config/main.xml
  containments/desktop/package/contents/ui/FolderView.qml
  containments/desktop/package/contents/ui/FolderViewLayer.qml
  containments/desktop/plugins/folder/CMakeLists.txt
  containments/desktop/plugins/folder/autotests/CMakeLists.txt
  containments/desktop/plugins/folder/autotests/foldermodeltest.cpp
  containments/desktop/plugins/folder/autotests/foldermodeltest.h
  containments/desktop/plugins/folder/autotests/screenmappertest.cpp
  containments/desktop/plugins/folder/autotests/screenmappertest.h
  containments/desktop/plugins/folder/foldermodel.cpp
  containments/desktop/plugins/folder/foldermodel.h
  containments/desktop/plugins/folder/folderplugin.cpp
  containments/desktop/plugins/folder/screenmapper.cpp
  containments/desktop/plugins/folder/screenmapper.h

To: amantia, #plasma, ervin, mlaurent, dvratil, hein, aacid, davidedmundson, 
apol, mwolff
Cc: anthonyfieroni, ngraham, mwolff, davidedmundson, broulik, mart, 
plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol