D26958: [Kickoff] Don't enable OpacityMask layer.effect in software mode

[Kai Uwe Broulik]

This revision was automatically updated to reflect the committed changes.
Closed by commit R119:7a727fde4cb9: [Kickoff] Dont enable OpacityMask 
layer.effect in software mode (authored by broulik).

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26958?vs=74482=74483

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

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

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

D26958: [Kickoff] Don't enable OpacityMask layer.effect in software mode

[Kai Uwe Broulik]

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

REVISION SUMMARY
  It won't render otherwise. The code tried to check but only disabled the 
outline, not the actual effect.

TEST PLAN
  5.18
  
  - Ran `QT_QUICK_BACKEND=software plasmashell` and got my user shown in 
kickoff. In a square rather than a circle but better than not rendering the 
avatar at all

REPOSITORY
  R119 Plasma Desktop

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

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

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

D26953: Replace Ice Cold with Volna for Plasma 5.18

[Kai Uwe Broulik]

broulik added a comment.


  What's with the odd dithering, though? Is that intentional?
  
  FTR at 3840x2160 our past images were:
  5.16 (ice cold) 3.2 MiB
  5.14 (the one with the galaxy) 3.9 MiB
  5.12 (the blue-brown stripe) 3.2 MiB
  5.10 2.3 MiB
  5.9 (canoppee with the "9") a whopping 5.9 MiB
  
  So this one doesn't seem too excessive? Compared to ice cold, maybe, but not 
compared to the ones before it imho.

REPOSITORY
  R31 Breeze

BRANCH
  icecold-to-volna (branched from Plasma/5.18)

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

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

D26943: [Notifications] Set transient parent for file menu

[Kai Uwe Broulik]

This revision was automatically updated to reflect the committed changes.
Closed by commit R120:3c31b7a6a1b7: [Notifications] Set transient parent for 
file menu (authored by broulik).

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26943?vs=74436=74458

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

AFFECTED FILES
  applets/notifications/filemenu.cpp

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

D26943: [Notifications] Set transient parent for file menu

[Kai Uwe Broulik]

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

REVISION SUMMARY
  Required on Wayland so it can be properly positioned.
  
  BUG: 387597
  FIXED-IN: 5.18.0

TEST PLAN
  Copied a file on Wayland, right-clicked the proxy icon: menu is shown 
properly now

REPOSITORY
  R120 Plasma Workspace

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

AFFECTED FILES
  applets/notifications/filemenu.cpp

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

D26941: [Task Manager] Remove strict URL handling

[Kai Uwe Broulik]

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

REVISION SUMMARY
  The code tried hard to ignore garbage URLs, as Qt is quite lenient, e.g. 
`QUrl("Garbage Url")` is still valid.
  There is no way to change the strictness of a `QUrl` after creation, so the 
code would enforce it by doing `QUrl strictUrl(inputUrl.toString(), 
QUrl::StrictMode)`.
  However, `toString()` defaults to `PrettyDecoded` which avoids 
percent-encoding and keeps spaces in tact which is not a valid thing to have in 
a strict URL.
  Effectively, we want to ensure a URL is either a valid path to a local file, 
or one of the special `applications` (for menu ids), or `preferred` for 
preferred applications, like web browser,
  
  BUG: 385727
  FIXED-IN: 5.18.0

TEST PLAN
  Is this still good for 5.18?
  
  - Unit tests still pass
  - Comes with a new one to verify adding desktop entries with spaces works
  - Pinned an application which had spaces in its desktop file to the task bar:
- Was successfully added with the menu
- Was successfully merged with the window, if open
- Was successfully restored/loaded on plasmashell restart
- Was successfully removed with the context menu
  - Created a new activity, pinned an application which had spaces in its 
desktop file to only one activity:
- Was successfully restored/hidden when switching activities
- all of the above

REPOSITORY
  R120 Plasma Workspace

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

AFFECTED FILES
  libtaskmanager/autotests/launchertasksmodeltest.cpp
  libtaskmanager/launchertasksmodel.cpp
  libtaskmanager/launchertasksmodel_p.h

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

D26881: [Applets/Folder View] Allow using a folder that ends with a space

[Kai Uwe Broulik]

broulik added a comment.


  That's somewhat oddly spcific, isn't it? I'd be surprised that 
`fromLocalFile` strips trailing spaces? :0

REPOSITORY
  R119 Plasma Desktop

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

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

D26896: Kill the KinfoCenter binary

[Kai Uwe Broulik]

broulik added a comment.


  Would it make sense to keep a `kinfocenter` script that just launchers 
`systemsettings5 -i` for compat?
  Does it still show up if you type "kinfocenter" into KRunner, not sure if it 
uses the `Exec` for that if this name is in the desktop file somewhere other 
than that.

REPOSITORY
  R102 KInfoCenter

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

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

D26895: Introduce an Info Center mode

[Kai Uwe Broulik]

broulik added a comment.


  Great idea!

INLINE COMMENTS

> SettingsBase.h:100
>  KAboutApplicationDialog * aboutDialog = nullptr;
> +bool m_infoCenterMode = false;
>  };

I would prefer this to be an enum

> main.cpp:66
> +if (infoCenterMode) {
> +QCoreApplication::setApplicationName(QStringLiteral("kinfocenter"));
> +application.setApplicationName(QStringLiteral("kinfocenter"));

Can we not just create a different `KAboutData` instance?

> main.cpp:71
> +aboutData.setDesktopFileName(QStringLiteral("org.kde.kinfocenter"));
> +aboutData.setShortDescription(QStringLiteral("Centralized and 
> convenient overview of system information"));
> +
> application.setWindowIcon(QIcon::fromTheme(QStringLiteral("hwinfo")));

`i18n`

REPOSITORY
  R124 System Settings

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

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

D26850: [SystemTray] Refresh icon in settings on update

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> systemtraymodel.cpp:269
> +QHash roleNames = sourceModel->roleNames();
> +for (const int key : roleNames.keys()) {
> +if (!m_roleNames.contains(key)) {

Avoid `keys()` which creates a temporary list, instead use iterators

  const auto roleNames = sourceModel->roleNames();
  for (auto it = roleNames.begin(), end = roleNames.end(); it != end; ++it) {
  if (!m_roleNames.contains(it.key()) {
  m_roleNames.insert(it.key(), it.value());
  }
  }

REPOSITORY
  R120 Plasma Workspace

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

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

D26797: KCM/Component Refactor UI to a single list of combobox

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> componentchooser.cpp:65
>  
> - if (loadedConfigWidget) {
> - configWidgetMap.insert(service, loadedConfigWidget);
> - configContainer->addWidget(loadedConfigWidget);
> - connect(loadedConfigWidget, SIGNAL(changed(bool)), this, 
> SLOT(emitChanged(bool)));
> - }
> -}
> +QLabel *label = new QLabel(i18nc("The label for the combobox : 
> browser, terminal emulator...)", "%1:", name), this);
> +label->setToolTip(cfg.group(QByteArray()).readEntry("Comment", 
> QString()));

Superfluous space before `:`

> componentchooser.cpp:70
>  
> - if (somethingChanged) {
> - if (KMessageBox::questionYesNo(this,i18n("You changed the 
> default component of your choice, do want to save that change now 
> ?"),QString(),KStandardGuiItem::save(),KStandardGuiItem::discard())==KMessageBox::Yes)
>  save();
> - }
> - const QString  = it->data(Qt::UserRole).toString();
> - KConfig cfg(service, KConfig::SimpleConfig);
> +connect(loadedConfigWidget, SIGNAL(changed(bool)), this, 
> SLOT(emitChanged()));
>  

Use new connect syntax

> componentchooser.cpp:93
> +}
> +loadedConfigWidget->setSizeAdjustPolicy(QComboBox::AdjustToContents);
>  

Check if `loadedConfigWidget` is null

> componentchooser.cpp:101
> +bool somethingChanged = false;
> +bool isDefault = true;
> +// check if another plugin has changed and default status

`isDefaults`

> componentchooser.cpp:103
> +// check if another plugin has changed and default status
> +for (auto it = configWidgetMap.constKeyValueBegin(); it != 
> configWidgetMap.constKeyValueEnd(); ++it) {
> +CfgPlugin *plugin = dynamic_cast((*it).second);

You don't actually use the `key`, so you can just use range-for.

> componentchooser.cpp:121
>  void ComponentChooser::load() {
> - if( configWidget )
> - {
> - CfgPlugin * plugin = dynamic_cast( configWidget );
> - if( plugin )
> - {
> - KConfig cfg(latestEditedService, KConfig::SimpleConfig);
> - plugin->load(  );
> - }
> +for (auto it = configWidgetMap.constKeyValueBegin(); it != 
> configWidgetMap.constKeyValueEnd(); ++it) {
> +

normal `constBegin()` and then `it.key()` and `it.value()` below

> componentchooser.cpp:141
> +CfgPlugin *plugin = dynamic_cast( widget );
> +if (plugin) {
> +KConfig cfg(service, KConfig::SimpleConfig);

In some places you check if your `dynamic_cast` worked and sometimes you don't. 
Since we only put `CfgPlugin` instanes in there, I don't think we need to 
check. See my note on the ominous plug-in architecture above.

> componentchooser.cpp:152
>  void ComponentChooser::restoreDefault() {
> -if (configWidget)
> -{
> +for (const auto  : configWidgetMap) {
>  dynamic_cast(configWidget)->defaults();

The hash contains pointers, so `auto *`

> componentchooser.cpp:161
> - {
> - loadedConfigWidget = new CfgComponent(configContainer);
> - 
> static_cast(loadedConfigWidget)->ChooserDocu->setText(i18n("Choose
>  from the list below which component should be used by default for the %1 
> service.", name));

It appears you dropped the generic component configuration? If that even was a 
thing... makes me wonder why this KCM uses desktop files when it effectively 
only operates on a fixed set of options.

> It will be parted of the plugin interface which I plan for KDE 3.2.

Well, so much for that I guess?

> componentchooserbrowser.cpp:104
>  }
>  if (service->storageId() == 
> QStringLiteral("org.kde.falkon.desktop")) {
> +m_falkonIndex = count() - 1;

As a future step I would like those default components not hardcoded in the code

> componentchooserbrowser.cpp:111
>  // we have a browser specified by the user
> -
> browserCombo->addItem(QIcon::fromTheme(QStringLiteral("application-x-shellscript")),
>  browser->name(), browser->storageId());
> -browserCombo->setCurrentIndex(browserCombo->count() - 1);
> -m_currentIndex = browserCombo->count() - 1;
> +
> addItem(QIcon::fromTheme(QStringLiteral("application-x-shellscript")), 
> browser->name(), browser->storageId());
> +setCurrentIndex(count() - 1);

Something funky is going on with this one:

I configured a custom `kdialog --sorry "%f"` command line. It only shows up as 
"kdialog" so I can't tell what the actual command was and not edit it later.

More importantly, though, I then changed my browser to be Kate. While it 
initially showed "Kate" with proper icon, when I re-open the KCM, it shows 
"kdialog" again as custom command, while still opening Kate when I open http 
URLs. 
Now it keeps showing "kdialog" no matter what default browser I use.

> componentchooserbrowser.cpp:117
>  // add a other option to add a new browser
> -
> 

D26842: Fix fonts KCM button state

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> fonts.cpp:479
> +defaultState.dpi = 0;
> +defaultState.subPixel = KXftConfig::SubPixel::Rgb;
> +defaultState.hinting = KXftConfig::Hint::Slight;

Kinda defies the purpose of using kconfigxt if we end up hardcoding the state 
in code again?

REPOSITORY
  R119 Plasma Desktop

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

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

D26806: [Applets/Power Manager] Update layout based on T10470

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> davidedmundson wrote in BatteryItem.qml:64
> You can't do that.
> 
> That'll change the column size, which will change our text bounding box which 
> will change our paintedWidth. A loop.

That's been in there ever since the initial release 2013.

REPOSITORY
  R120 Plasma Workspace

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

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

D26862: Remove unused includes

[Kai Uwe Broulik]

This revision was automatically updated to reflect the committed changes.
Closed by commit R102:5a5f32ea02f8: Remove unused includes (authored by 
broulik).

REPOSITORY
  R102 KInfoCenter

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26862?vs=74197=74219

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

AFFECTED FILES
  Modules/base/info_linux.cpp
  Modules/info/info.cpp

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

D26825: Bind gtk-enable-animations setting to global animation speed slider

[Kai Uwe Broulik]

This revision was automatically updated to reflect the committed changes.
Closed by commit R99:66ab37df7aed: Bind gtk-enable-animations setting to global 
animation speed slider (authored by broulik).

REPOSITORY
  R99 KDE Gtk Configuration Tool

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26825?vs=74203=74207

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

AFFECTED FILES
  kded/configeditor.cpp
  kded/configeditor.h
  kded/configvalueprovider.cpp
  kded/configvalueprovider.h
  kded/gtkconfig.cpp
  kded/gtkconfig.h

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

D26825: Bind gtk-enable-animations setting to global animation speed slider

[Kai Uwe Broulik]

broulik updated this revision to Diff 74203.
broulik added a comment.


  - Fix typos

REPOSITORY
  R99 KDE Gtk Configuration Tool

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26825?vs=74193=74203

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

AFFECTED FILES
  kded/configeditor.cpp
  kded/configeditor.h
  kded/configvalueprovider.cpp
  kded/configvalueprovider.h
  kded/gtkconfig.cpp
  kded/gtkconfig.h

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

D26862: Remove unused includes

[Kai Uwe Broulik]

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

TEST PLAN
  - Compiles

REPOSITORY
  R102 KInfoCenter

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

AFFECTED FILES
  Modules/base/info_linux.cpp
  Modules/info/info.cpp

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

D26825: Bind gtk-enable-animations setting to global animation speed slider

[Kai Uwe Broulik]

broulik updated this revision to Diff 74193.

REPOSITORY
  R99 KDE Gtk Configuration Tool

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26825?vs=74115=74193

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

AFFECTED FILES
  kded/configeditor.cpp
  kded/configeditor.h
  kded/configvalueprovider.cpp
  kded/configvalueprovider.h
  kded/gtkconfig.cpp
  kded/gtkconfig.h

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

D26640: [applets/weather] Port weather station picker to QQC2+ListView

[Kai Uwe Broulik]

broulik added a comment.


  Using `Kirigami.SearchField` would be nice, so Ctrl+F acts as a shortcut for 
getting back to the search field when focus jumped to the list.
  
  (And can we later improve that weird workflow where no providers are selected 
and the Search field is disabled for no immediately obvious reason? :)

INLINE COMMENTS

> WeatherStationPicker.qml:39
>  source = "";
> -locationListView.forceActiveFocus();
> -
>  locationListModel.searchLocations(searchStringEdit.text, 
> selectedServices);
>  }

You no longer hide the `noSearchResultsReport` label when a new query starts, 
awkwardly overlapping the busy indicator until the new one completes.

> WeatherStationPicker.qml:91
>  Layout.minimumWidth: implicitWidth
> +focus: true
>  placeholderText: i18nc("@info:placeholder", "Enter location")

Maybe explicitly focus this it when the dialog opens again otherwise on 
subsequent opening of the dialog it will instead have whatever item was focused 
previously?

> WeatherStationPicker.qml:148
> +delegate: QQC2.ItemDelegate {
> +id: sourceDelegate
> +width: locationListView.width

Unused `id`

> WeatherStationPicker.qml:165
> +verticalAlignment: Text.AlignVCenter
> +text: i18nc("@info", "No weather stations found for '%1'", 
> searchStringEdit.text);
> +wrapMode: Text.WordWrap

This should be set programatically when the query finishes like before instead 
of a binding. Otherwise you'll end up in the weird situation where it says 
"Nothing found for `foo`" and then you start typing a new query and it live 
updates to "Nothing found for `bar`" even though you didn't actually launch a 
new query

> ngraham wrote in WeatherStationPicker.qml:144
> I copied it from the notifications KCM :)

The notifications KCM doesn't wrap, though?

REPOSITORY
  R114 Plasma Addons

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

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

D26784: KCM KDED: Add immutability and fix default, reset, apply buttons

[Kai Uwe Broulik]

broulik added a comment.


  Not a fan of the `AutoloadEnabledSavedRole` in the model

INLINE COMMENTS

> kcmkded.cpp:269
> +if (!idx.data(ModulesModel::ImmutableRole).toBool()) {
> +m_model->setData(idx, true, ModulesModel::AutoloadEnabledRole);
>  }

I think instead you could have the model refuse to `setData` when immutable

> modulesmodel.cpp:128
>  const bool autoloadEnabled = value.toBool();
>  if (item.type == KDEDConfig::AutostartType
> +&& item.autoloadEnabled != autoloadEnabled) {

Since generally `setData` only does things for `AutostartType`, perhaps move 
that check out of the `switch`

> modulesmodel.cpp:235
>  const bool autoloadEnabled = cg.readEntry("autoload", true);
> +const bool immutable  = cg.isEntryImmutable("autoload");
>  

Superfluous space

> modulesmodel.cpp:275
>  endResetModel();
> +emit autoloadedModulesChanged();
>  }

Check whether it actually changed before emitting it

REPOSITORY
  R119 Plasma Desktop

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

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

D26825: Bind gtk-enable-animations setting to global animation speed slider

[Kai Uwe Broulik]

broulik updated this revision to Diff 74115.
broulik added a comment.


  - Drop pointless argument

REPOSITORY
  R99 KDE Gtk Configuration Tool

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26825?vs=74108=74115

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

AFFECTED FILES
  kded/configvalueprovider.cpp
  kded/configvalueprovider.h
  kded/gtkconfig.cpp
  kded/gtkconfig.h

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

D26825: Bind gtk-enable-animations setting to global animation speed slider

[Kai Uwe Broulik]

broulik updated this revision to Diff 74108.
broulik added a comment.


  - Drop superfluous `reparseConfiguration` call

REPOSITORY
  R99 KDE Gtk Configuration Tool

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26825?vs=74094=74108

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

AFFECTED FILES
  kded/configvalueprovider.cpp
  kded/configvalueprovider.h
  kded/gtkconfig.cpp
  kded/gtkconfig.h

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

D26834: libnotificationmanager : deprecate Settings ctor that takes a config

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> settings.cpp:40
>  
> +const static QString s_configFile { QStringLiteral("plasmanotifyrc") };
> +

Global statics are to be avoided in libraries

> settings.cpp:183
>  
> +Settings::Settings(const KSharedConfig::Ptr &, QObject *parent)
> +: Settings(parent)

I prefer to have a name and then explicitly `Q_UNUSED` in the function body

REPOSITORY
  R120 Plasma Workspace

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

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

D26825: Bind gtk-enable-animations setting to global animation speed slider

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> davidedmundson wrote in configvalueprovider.cpp:146
> You don't need this.
> 
> KConfigWatcher does it automagically on change.
> 
> I did that because I wanted a way for N connections to only reparse the file 
> once. It also means you can use the config watcher with no signal handling, 
> just install and you get the correct values on the next read. Amazing.
> 
> My fault for not having enough docs

I know, I just kept it consistent with the other code. Let's clean up this 
everywhere else above separately?

REPOSITORY
  R99 KDE Gtk Configuration Tool

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

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

D26825: Bind gtk-enable-animations setting to global animation speed slider

[Kai Uwe Broulik]

broulik updated this revision to Diff 74094.
broulik added a comment.


  - Fix settings key

REPOSITORY
  R99 KDE Gtk Configuration Tool

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26825?vs=74080=74094

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

AFFECTED FILES
  kded/configvalueprovider.cpp
  kded/configvalueprovider.h
  kded/gtkconfig.cpp
  kded/gtkconfig.h

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

D26825: Bind gtk-enable-animations setting to global animation speed slider

[Kai Uwe Broulik]

broulik updated this revision to Diff 74080.
broulik added a comment.


  - Add GTK2 key

REPOSITORY
  R99 KDE Gtk Configuration Tool

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26825?vs=74077=74080

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

AFFECTED FILES
  kded/configvalueprovider.cpp
  kded/configvalueprovider.h
  kded/gtkconfig.cpp
  kded/gtkconfig.h

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

D26825: Bind gtk-enable-animations setting to global animation speed slider

[Kai Uwe Broulik]

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

REVISION SUMMARY
  When it is set to "Instant", turn off GTK animations.

TEST PLAN
  - Started gedit, clicked "Open", got it flying in animated.
  - Changed slider to Instant in workspace KCM, restarted gedit, clicked 
"Open", got it show immediately
  - Settings change doesn't appear to work at runtime with gtk3 but that's 
probably unrelated

REPOSITORY
  R99 KDE Gtk Configuration Tool

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

AFFECTED FILES
  kded/configvalueprovider.cpp
  kded/configvalueprovider.h
  kded/gtkconfig.cpp
  kded/gtkconfig.h

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

D26726: [Breeze Cursors] Add some more cursor names

[Kai Uwe Broulik]

This revision was automatically updated to reflect the committed changes.
Closed by commit R31:419274f46a67: [Breeze Cursors] Add some more cursor names 
(authored by broulik).

REPOSITORY
  R31 Breeze

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26726?vs=73760=74076

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

AFFECTED FILES
  cursors/Breeze/Breeze/cursors/ew-resize
  cursors/Breeze/Breeze/cursors/nesw-resize
  cursors/Breeze/Breeze/cursors/ns-resize
  cursors/Breeze/Breeze/cursors/nwse-resize
  cursors/Breeze/src/cursorList
  cursors/Breeze_Snow/Breeze_Snow/cursors/ew-resize
  cursors/Breeze_Snow/Breeze_Snow/cursors/nesw-resize
  cursors/Breeze_Snow/Breeze_Snow/cursors/ns-resize
  cursors/Breeze_Snow/Breeze_Snow/cursors/nwse-resize
  cursors/Breeze_Snow/src/cursorList

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

D26494: Runner/Windows make the window finding more reliable

[Kai Uwe Broulik]

broulik added a comment.


  Sorry, I don't know how any of that prepare and teardown stuff works, so I 
can't really judge.

REPOSITORY
  R120 Plasma Workspace

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

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

D26047: Expose KConfig settings to allow registration in KCM Notification

[Kai Uwe Broulik]

broulik added a comment.


  Other than the seemingly missing `config`, looks good

INLINE COMMENTS

> settings.cpp:173
> -if (!s_settingsInited) {
> -DoNotDisturbSettings::instance(config);
> -NotificationSettings::instance(config);

I still want to be able to specify what `config` (constructor argument) to use 
for autotests

REPOSITORY
  R120 Plasma Workspace

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

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

D26531: [Wallpaper] Show author as subtitle in configuration

[Kai Uwe Broulik]

This revision was automatically updated to reflect the committed changes.
Closed by commit R120:3729873fdd1d: [Wallpaper] Show author as subtitle in 
configuration (authored by broulik).

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26531?vs=73094=74018

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

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

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

D26803: [Notifications] Send reply text as targeted signal

[Kai Uwe Broulik]

This revision was automatically updated to reflect the committed changes.
Closed by commit R120:5369cf775c4b: [Notifications] Send reply text as targeted 
signal (authored by broulik).

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26803?vs=74006=74009

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

AFFECTED FILES
  libnotificationmanager/notification.cpp
  libnotificationmanager/notification.h
  libnotificationmanager/notification_p.h
  libnotificationmanager/notificationsmodel.cpp
  libnotificationmanager/server.cpp
  libnotificationmanager/server.h
  libnotificationmanager/server_p.cpp
  libnotificationmanager/server_p.h

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

D26796: [Notifications] When there is only a reply action, show reply field right away

[Kai Uwe Broulik]

This revision was automatically updated to reflect the committed changes.
Closed by commit R120:3328f6a2f5ad: [Notifications] When there is only a reply 
action, show reply field right away (authored by broulik).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D26796?vs=73998=74010#toc

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26796?vs=73998=74010

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

AFFECTED FILES
  applets/notifications/package/contents/ui/NotificationItem.qml
  applets/notifications/package/contents/ui/NotificationReplyField.qml

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

D26803: [Notifications] Send reply text as targeted signal

[Kai Uwe Broulik]

broulik updated this revision to Diff 74006.
broulik added a comment.


  - No fallback, `createTargetedSignal` without a service seems to behave like 
the non-targeted version

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26803?vs=73999=74006

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

AFFECTED FILES
  libnotificationmanager/notification.cpp
  libnotificationmanager/notification.h
  libnotificationmanager/notification_p.h
  libnotificationmanager/notificationsmodel.cpp
  libnotificationmanager/server.cpp
  libnotificationmanager/server.h
  libnotificationmanager/server_p.cpp
  libnotificationmanager/server_p.h

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

D26803: [Notifications] Send reply text as targeted signal

[Kai Uwe Broulik]

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

REVISION SUMMARY
  Avoids waking up non-interested parties.

TEST PLAN
  5.18
  while it is an API change, it was only released in the Beta yet
  
  - Tried with Telgram patch, reply text field still worked

REPOSITORY
  R120 Plasma Workspace

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

AFFECTED FILES
  libnotificationmanager/notification.cpp
  libnotificationmanager/notification.h
  libnotificationmanager/notification_p.h
  libnotificationmanager/notificationsmodel.cpp
  libnotificationmanager/server.cpp
  libnotificationmanager/server.h
  libnotificationmanager/server_p.cpp
  libnotificationmanager/server_p.h

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

D26796: [Notifications] When there is only a reply action, show reply field right away

[Kai Uwe Broulik]

broulik updated this revision to Diff 73998.
broulik added a comment.


  - Move `MouseArea` inside as to not break the `RowLayout`

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26796?vs=73972=73998

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

AFFECTED FILES
  applets/notifications/package/contents/ui/NotificationItem.qml
  applets/notifications/package/contents/ui/NotificationReplyField.qml

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

D26796: [Notifications] When there is only a reply action, show reply field right away

[Kai Uwe Broulik]

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

REVISION SUMMARY
  Makes it more obvious that there is an inline reply rather than having a 
"Reply" button where the user must guess what it'll do.

TEST PLAN
  Since quick reply isn't really used anywhere yet I would like to have this in 
5.18
  
  - Sent notification with only inline-reply action, got text field right away, 
clicking it focused the text field properly
  - Sent notification with inline-reply + another action, got two buttons, 
inline-reply button revealed text field and focused it properly autoamtically
  - Verified that "default" action did not count as an action in this 
calculation

REPOSITORY
  R120 Plasma Workspace

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

AFFECTED FILES
  applets/notifications/package/contents/ui/NotificationItem.qml

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

D26640: [applets/weather] Port weather station picker to QQC2+ListView

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> WeatherStationPicker.qml:44
>  if (!success) {
>  noSearchResultReport.text = i18nc("@info", "No weather stations 
> found for '%1'", searchString);
>  noSearchResultReport.visible = true;

Given the item is hidden anyway, you can probably assign this as a binding 
right away

> WeatherStationPicker.qml:134
> +background.visible = true;
> +searchStringEdit.forceActiveFocus();
>  }

Does `focus: true` on the `TextField` instead of the `ListView` make this 
redundant?

> WeatherStationPicker.qml:144
> +keyNavigationEnabled: true
> +keyNavigationWraps: true
> +

This is unlike any other list we have in settings?

> WeatherStationPicker.qml:169
> +wrapMode: Text.WordWrap
> +visible: locationListView.count === 0 && 
> searchStringEdit.length > 0
> +enabled: false

You're constantly breaking this binding by assigning to it elsewhere 
programmatically.

REPOSITORY
  R114 Plasma Addons

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

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

D26726: [Breeze Cursors] Add some more cursor names

[Kai Uwe Broulik]

broulik added a comment.


  > even though they're still "up for discussion."
  
  Yeah, and then I looked at the date ;)

REPOSITORY
  R31 Breeze

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

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

D26726: [Breeze Cursors] Add some more cursor names

[Kai Uwe Broulik]

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

REVISION SUMMARY
  They are "up for discussion" [1] but seem to be used by e.g. Chrome.
  
  [1] https://www.freedesktop.org/wiki/Specifications/cursor-spec/

TEST PLAN
  Juts a bunch of symlinks, so 5.18
  Not sure where that `cursorList` file is being used (I also added some others 
that weren't added to this file in eb3498d9a797e3e21f0e722ecc0c9507c6f1b8ea 

  
  Now works
  F7888108: Screenshot_20200117_134406.PNG 


REPOSITORY
  R31 Breeze

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

AFFECTED FILES
  cursors/Breeze/Breeze/cursors/ew-resize
  cursors/Breeze/Breeze/cursors/nesw-resize
  cursors/Breeze/Breeze/cursors/ns-resize
  cursors/Breeze/Breeze/cursors/nwse-resize
  cursors/Breeze/src/cursorList
  cursors/Breeze_Snow/Breeze_Snow/cursors/ew-resize
  cursors/Breeze_Snow/Breeze_Snow/cursors/nesw-resize
  cursors/Breeze_Snow/Breeze_Snow/cursors/ns-resize
  cursors/Breeze_Snow/Breeze_Snow/cursors/nwse-resize
  cursors/Breeze_Snow/src/cursorList

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

D26330: [MPRIS Data Engine] Ignore players with CanControl false in multiplexer

[Kai Uwe Broulik]

broulik abandoned this revision.
broulik added a comment.


  Superseded by D26702 

REPOSITORY
  R120 Plasma Workspace

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

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

D26689: [MPRIS Data Engine] Refactor to reduce code duplication

[Kai Uwe Broulik]

broulik abandoned this revision.
broulik added a comment.


  Incorporated in D26702 

REPOSITORY
  R120 Plasma Workspace

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

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

D26720: Cache single image wallpapers locally

[Kai Uwe Broulik]

broulik added a comment.


  This doesn't appear to be containment-aware (e.g. multi-screen, activities, 
etc).
  How does the settings page handle this scenario?

REPOSITORY
  R120 Plasma Workspace

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

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

D26718: autostart KRunner with Plasma, aggregate text for KRunner in DesktopView

[Kai Uwe Broulik]

broulik added a comment.


  Hmm, I'd appreciate some startup time optimizations for KRunner first.
  Maybe if we autostarted KRunner based on whether it was used previously would 
be a nice trade-off between resources for systems that don't use it and power 
users that always use it and get annoyed by the lag.

INLINE COMMENTS

> main.cpp:231
> +if (krunnerPeer->isValid()) {
> +krunnerPeer->asyncCall(QStringLiteral("Ping"));
> +}

This is not how we want to autostart something :) Instead, you want to place 
the desktop file in autostart folder again

REPOSITORY
  R120 Plasma Workspace

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

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

D26710: [Notifications] Support DESKTOP_FILE_HINT without BAMF prefix

[Kai Uwe Broulik]

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

REVISION SUMMARY
  No idea why this has changed for my Spotify but that's what I have now...

TEST PLAN
  5.18
  My spotify track changes are associated with Spotify again

REPOSITORY
  R120 Plasma Workspace

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

AFFECTED FILES
  libnotificationmanager/utils.cpp

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

D26709: add gpu entry to about system

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> GPUEntry.cpp:49
> +// It seems the renderer value may have excess information in 
> parentheses ->
> +// strip that. Elide would probably be nicer, a bit meh with 
> QWidgets though.
> +value = value.mid(0, value.indexOf('('));

`KSqueezedTextLabel` doesn't help?

REPOSITORY
  R102 KInfoCenter

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

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

D26706: Remove notification inhibitor lock

[Kai Uwe Broulik]

This revision was automatically updated to reflect the committed changes.
Closed by commit R116:8df6c01ca46f: Remove notification inhibitor lock 
(authored by broulik).

REPOSITORY
  R116 Plasma Network Management Applet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26706?vs=73701=73706

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

AFFECTED FILES
  applet/contents/ui/PopupDialog.qml

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

D26506: [KDED KCM] Rewrite as KDeclarative ScrollViewKCM

[Kai Uwe Broulik]

This revision was automatically updated to reflect the committed changes.
Closed by commit R119:aa2119a835e9: [KDED KCM] Rewrite as KDeclarative 
ScrollViewKCM (authored by broulik).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D26506?vs=73369=73705#toc

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26506?vs=73369=73705

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

AFFECTED FILES
  CMakeLists.txt
  kcms/kded/CMakeLists.txt
  kcms/kded/filterproxymodel.cpp
  kcms/kded/filterproxymodel.h
  kcms/kded/kcmkded.cpp
  kcms/kded/kcmkded.desktop
  kcms/kded/kcmkded.h
  kcms/kded/modulesmodel.cpp
  kcms/kded/modulesmodel.h
  kcms/kded/package/contents/ui/main.qml
  kcms/kded/package/metadata.desktop

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

D26706: Remove notification inhibitor lock

[Kai Uwe Broulik]

broulik created this revision.
broulik added reviewers: Plasma, jgrulich.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
Herald added 1 blocking reviewer(s): jgrulich.
broulik requested review of this revision.

REVISION SUMMARY
  Now that the notifications dodge the popup

TEST PLAN
  5.18
  
  - No suspicious warnings
  - Search field still closes when closing the popup

REPOSITORY
  R116 Plasma Network Management Applet

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

AFFECTED FILES
  applet/contents/ui/PopupDialog.qml

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

D26702: [MPRIS Data Engine] Support player proxying for another one

[Kai Uwe Broulik]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit R120:e4f91a3ee8b0: [MPRIS Data Engine] Support player proxying 
for another one (authored by broulik).

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26702?vs=73678=73692

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

AFFECTED FILES
  dataengines/mpris2/multiplexer.cpp
  dataengines/mpris2/multiplexer.h

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

D26684: [Notifications KCM] Add docbook for new KCM

[Kai Uwe Broulik]

This revision was automatically updated to reflect the committed changes.
Closed by commit R119:75481705c68e: [Notifications KCM] Add docbook for new KCM 
(authored by broulik).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D26684?vs=73618=73681#toc

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26684?vs=73618=73681

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

AFFECTED FILES
  doc/kcontrol/notifications/CMakeLists.txt
  doc/kcontrol/notifications/index.docbook

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

D26702: [MPRIS Data Engine] Support player proxying for another one

[Kai Uwe Broulik]

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

REVISION SUMMARY
  Plasma Browser Integration provides controls for Chrome, so when a player 
with `kde:pid` metadata is found, it is preferred over the actual pid it 
represents.

TEST PLAN
  Still behaves as before:
  
  - It never switches away from a playing player when another starts playing
  - It switches to a playing player if there was none before
  - When current player gets paused, it switches to another playing one, if 
any, or a paused one, if any, or a stopped one, if any. When the current player 
gets paused and there is no playing one, it makes sure to prefer the paused 
proxy over another random paused player.
  - Closing a player, switches to another play one, if any, or a paused one, if 
any, or a stopped one, if any
  - Verified that it never used Chrome's broken mpris when p-b-i was running
  - When disabling p-b-i controls were transferred to Chrome's own mpris

REPOSITORY
  R120 Plasma Workspace

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

AFFECTED FILES
  dataengines/mpris2/multiplexer.cpp
  dataengines/mpris2/multiplexer.h

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

D26689: [MPRIS Data Engine] Refactor to reduce code duplication

[Kai Uwe Broulik]

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

REVISION SUMMARY
  `addPlayer` and `playerUpdated` effectively do the same, so merge the two 
implementations.
  Also simplify nesting in `setPlayerActive`

TEST PLAN
  Still behaves as before:
  
  - It never switches away from a playing player when another starts playing
  - It switches to a playing player if there was none before
  - When current player gets paused, it switches to another playing one, if 
any, or a paused one, if any, or a stopped one, if any
  - Closing a player, switches to another play one, if any, or a paused one, if 
any, or a stopped one, if any

REPOSITORY
  R120 Plasma Workspace

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

AFFECTED FILES
  dataengines/mpris2/multiplexer.cpp
  dataengines/mpris2/multiplexer.h

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

D26684: [Notifications KCM] Add docbook for new KCM

[Kai Uwe Broulik]

broulik updated this revision to Diff 73618.
broulik added a comment.


  - Fix wording

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26684?vs=73600=73618

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

AFFECTED FILES
  doc/kcontrol/CMakeLists.txt
  doc/kcontrol/kcmnotify/CMakeLists.txt
  doc/kcontrol/kcmnotify/index.docbook
  doc/kcontrol/notifications/CMakeLists.txt
  doc/kcontrol/notifications/index.docbook

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

D26684: [Notifications KCM] Add docbook for new KCM

[Kai Uwe Broulik]

broulik created this revision.
broulik added a reviewer: Documentation.
Herald added projects: Plasma, Documentation.
Herald added subscribers: kde-doc-english, plasma-devel.
broulik requested review of this revision.

REVISION SUMMARY
  BUG: 415250
  FIXED-IN: 5.18.0

TEST PLAN
  - Clicked "Help" button, got khelpcenter showing an up to date docbook

REPOSITORY
  R119 Plasma Desktop

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

AFFECTED FILES
  doc/kcontrol/CMakeLists.txt
  doc/kcontrol/kcmnotify/CMakeLists.txt
  doc/kcontrol/kcmnotify/index.docbook
  doc/kcontrol/notifications/CMakeLists.txt
  doc/kcontrol/notifications/index.docbook

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

D26162: [Notifications KCM] Force re-evaluation of position radio buttons

[Kai Uwe Broulik]

This revision was automatically updated to reflect the committed changes.
Closed by commit R119:db95a53cbdb6: [Notifications KCM] Force re-evaluation of 
position radio buttons (authored by broulik).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D26162?vs=72014=73599#toc

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26162?vs=72014=73599

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

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

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

D26583: [Application Style] Add GTK Application Style Page

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> gikari wrote in gtkthemesmodel.cpp:107
> Did you mean
> 
>   if (parent.isValid()) {
>   return 0;
>   }
>   return m_themesList.count();
> 
> ?

I think so :D

REPOSITORY
  R119 Plasma Desktop

BRANCH
  gtk-style-page

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

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

D26423: [Notifications KCM] Move Plasma Workspace "service" to the top of its category

[Kai Uwe Broulik]

This revision was automatically updated to reflect the committed changes.
Closed by commit R119:a0f16565015e: [Notifications] Move Plasma Workspace 
service to the top of its category (authored by broulik).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D26423?vs=73419=73508#toc

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26423?vs=73419=73508

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

AFFECTED FILES
  kcms/notifications/sourcesmodel.cpp

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

D26654: support mobile mode scrollbar

[Kai Uwe Broulik]

broulik added a comment.


  Cool! The width change during the slide animation looks a bit odd, though

INLINE COMMENTS

> ScrollBar.qml:50
> +id: handleGraphics
> +x: scrolling ? parent.width/2 - width/2 : parent.width - width
> +

`Math.round`

REPOSITORY
  R858 Qt Quick Controls 2: Desktop Style

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

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

D26622: RFC: [Notifications] Raise application window if no default action is provided

[Kai Uwe Broulik]

This revision was automatically updated to reflect the committed changes.
Closed by commit R120:48d3613aa843: [Notifications] Raise application window if 
no default action is provided (authored by broulik).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D26622?vs=73403=73497#toc

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26622?vs=73403=73497

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

AFFECTED FILES
  applets/notifications/package/contents/ui/NotificationPopup.qml
  applets/notifications/package/contents/ui/global/Globals.qml

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

D26627: Mark applications that play audio for small panels.

[Kai Uwe Broulik]

broulik accepted this revision.
broulik added a comment.


  Lovely, thanks.

REPOSITORY
  R119 Plasma Desktop

BRANCH
  audiosize (branched from master)

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

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

D26423: [Notifications KCM] Move Plasma Workspace "service" to the top of its category

[Kai Uwe Broulik]

broulik updated this revision to Diff 73419.
broulik retitled this revision from "[Notifications KCM] Move Plasma Workspace 
"service" to the top" to "[Notifications KCM] Move Plasma Workspace "service" 
to the top of its category".
broulik edited the summary of this revision.
broulik edited the test plan for this revision.
broulik added a comment.


  - Sort it only to the top of services category

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26423?vs=72769=73419

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

AFFECTED FILES
  kcms/notifications/sourcesmodel.cpp

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

D26624: [Workspace Behavior KCM] Add "animation speed" keyword

[Kai Uwe Broulik]

This revision was automatically updated to reflect the committed changes.
Closed by commit R119:6f1de8e8f0d3: [Workspace Behavior KCM] Add 
animation speed keyword (authored by broulik).

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26624?vs=73406=73415

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

AFFECTED FILES
  kcms/workspaceoptions/kcm_workspace.desktop

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

D26624: [Workspace Behavior KCM] Add "animation speed" keyword

[Kai Uwe Broulik]

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

REVISION SUMMARY
  Since that's where the animation speed slider is now in

REPOSITORY
  R119 Plasma Desktop

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

AFFECTED FILES
  kcms/workspaceoptions/kcm_workspace.desktop

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

D25925: Mark applications that play audio, for all task icon sizes

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> AudioStream.qml:92
> +// Do not display the indicator if the icons are too small.
> +visible: opacity > 0 && Math.min(iconBox.width, iconBox.height) >= 
> units.iconSizes.small
>  

This breaks displaying the icon on small panels like mine.

REPOSITORY
  R119 Plasma Desktop

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

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

D26622: RFC: [Notifications] Raise application window if no default action is provided

[Kai Uwe Broulik]

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

REVISION SUMMARY
  BUG: 416107

TEST PLAN
  - Clicked Thunderbird notification, got Thunderbird window raised
  - Opened new composer window, brought Thunderbird into the front, activated 
another window, Thunderbird window got raised and not the composer matching the 
stacking order

REPOSITORY
  R120 Plasma Workspace

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

AFFECTED FILES
  applets/notifications/package/contents/ui/NotificationPopup.qml
  applets/notifications/package/contents/ui/global/Globals.qml

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

D26120: [Wizard] Show notification instead of finished page when pairing succeeds

[Kai Uwe Broulik]

This revision was automatically updated to reflect the committed changes.
Closed by commit R97:5101d7902f9f: [Wizard] Show notification instead of 
finished page when pairing succeeds (authored by broulik).

REPOSITORY
  R97 Bluedevil

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26120?vs=71887=73396

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

AFFECTED FILES
  src/bluedevil.notifyrc
  src/wizard/CMakeLists.txt
  src/wizard/bluewizard.cpp
  src/wizard/pages/success.cpp

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

D25860: [plasmoid] Show device battery percentage

[Kai Uwe Broulik]

This revision was automatically updated to reflect the committed changes.
Closed by commit R97:3686a5acf6cf: [plasmoid] Show device battery percentage 
(authored by broulik).

REPOSITORY
  R97 Bluedevil

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25860?vs=71216=73397

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

AFFECTED FILES
  src/applet/package/contents/ui/DeviceItem.qml

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

D26516: [Icon Item] Check URL validity and scheme

[Kai Uwe Broulik]

This revision was automatically updated to reflect the committed changes.
Closed by commit R120:c765c990: [Icon Item] Check URL validity and scheme 
(authored by broulik).

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26516?vs=73038=73393

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

AFFECTED FILES
  applets/icon/iconapplet.cpp

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

D26531: [Wallpaper] Show author as subtitle in configuration

[Kai Uwe Broulik]

broulik added a comment.


  > can you look into that?
  
  Perhaps @ngraham can.

REPOSITORY
  R120 Plasma Workspace

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

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

Re: Long press global shortcuts

[Kai Uwe Broulik]

Hi,

I noticed on my phone all buttons (power, volume, home) react only on 
release.


On the desktop I don't know how annoying it would be to react only on 
press. However, that would also make it consistent with in-application 
shortcuts, which also trigger on press.


Perhaps it's best to only do on release triggers for shortcuts that 
actually have a long-press trigger?


Cheers
Kai Uwe

D26506: [KDED KCM] Rewrite as KDeclarative ScrollViewKCM

[Kai Uwe Broulik]

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


  - Remove enable/disable animation
  - Enabled becomes green
  - Now monitors kded running
  - Notifies when modules got automatically started/stopped when settings were 
saved

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26506?vs=73089=73369

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

AFFECTED FILES
  CMakeLists.txt
  kcms/kded/CMakeLists.txt
  kcms/kded/filterproxymodel.cpp
  kcms/kded/filterproxymodel.h
  kcms/kded/kcmkded.cpp
  kcms/kded/kcmkded.desktop
  kcms/kded/kcmkded.h
  kcms/kded/modulesmodel.cpp
  kcms/kded/modulesmodel.h
  kcms/kded/package/contents/ui/main.qml
  kcms/kded/package/metadata.desktop

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

D26587: [GTK3] Improve Electron menubar legibility

[Kai Uwe Broulik]

broulik added a comment.


  Hmm this makes inactive tabs even harder to spot than already :/

REPOSITORY
  R98 Breeze for Gtk

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

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

D26489: Use qmlRegisterAnonymousType

[Kai Uwe Broulik]

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


  Thanks

REPOSITORY
  R112 Milou

BRANCH
  arcpatch-D26489

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

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

D26547: pk: Load the AppStream database on a separate thread

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> PackageKitBackend.cpp:198
>  }
> +} else
> +ret.components << component;

Coding style ..

> PackageKitBackend.cpp:502
> +} else {
> +QTimer::singleShot(0, this, f);
> +}

Why is this delayed now?

REPOSITORY
  R134 Discover Software Store

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

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

D25449: Start of the new Formats KCM

[Kai Uwe Broulik]

broulik added a comment.


  I'm not a huge fan of that language list selection mode, it's unlike anything 
we use anywhere else.
  Most importantly, it makes the KCM take *forever* to load as it creates the 
entire list at once. Instead, you want to use the `ListView` properly so only 
that part scrolls.
  I also find it quite unclear which part I'm editing now. Also, keyboard 
navigation is missing there, i.e. I can't click on the language, start typing 
or use the arrow keys.
  Imho a custom `ComboBox` with a search field in its popup or something like 
that would be a better control.

INLINE COMMENTS

> formatsettings_impl.cpp:20
> +#include "formatsettings_impl.h"
> +#include "writeexports.h"
> +

This file is missing

> kcm.cpp:47
> +auto *about = new KAboutData(
> +QStringLiteral("kcm_formats"), i18n("Formats Configuration Module"),
> +QStringLiteral("0.1"), QString(), KAboutLicense::GPL,

Just `i18n("Formats")`? "configuration module" is quite geeky

> CountryList.qml:32
> +QQC2.Label {
> +text: i18n("Setting locale for ") + currentLocale.text
> +}

No word puzzzles:

  i18n("Setting locale for %1", currentLocale.text)

> main.qml:79
> +id: currency
> +text: i18n("Currency: ")
> +localeType: kcm.settings.monetary

Why the spaces?

> main.qml:83
> +onClicked: countryList.currentLocale = currency
> +onLocaleTypeChanged: kcm.settings.currency = 
> time.localeType
> +}

Copy paste error?

REPOSITORY
  R119 Plasma Desktop

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

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

D26531: [Wallpaper] Show author as subtitle in configuration

[Kai Uwe Broulik]

broulik added a comment.


  > "By" can also be tricky to translate into other languages
  
  That's why we have context. Currently it also has " by " 
explanation.

REPOSITORY
  R120 Plasma Workspace

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

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

D26531: [Wallpaper] Show author as subtitle in configuration

[Kai Uwe Broulik]

broulik added a comment.


  > However I see a problem with the delegate cell size:
  
  That's unrelated and something I would have expected was tested when this 
feature was added.

REPOSITORY
  R120 Plasma Workspace

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

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

D26530: ScrollView: Do not overlay scrollbars over contents

[Kai Uwe Broulik]

broulik added a comment.


  > every application developer needs to account for the scrollbars
  
  Says who? If the standard qqc2 desktop style item delegate and kirigami list 
delegate take it into account for their padding //reliably// you get 99% 
covered.
  
  > I was assuming mobile was out of scope.
  
  Well, there's also touch and convertibles, and I thought we wanted to be 
"responsive" in that sense. Might be for Kirigami, though, but I do think we 
use it on PlaMo as well.

REPOSITORY
  R858 Qt Quick Controls 2: Desktop Style

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

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

D26530: ScrollView: Do not overlay scrollbars over contents

[Kai Uwe Broulik]

broulik added a comment.


  I thought back then we explicitly did not want that?
  Also, how does this behave on mobile, ie. "transient scrollbars"?

REPOSITORY
  R858 Qt Quick Controls 2: Desktop Style

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

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

D26531: [Wallpaper] Show author as subtitle in configuration

[Kai Uwe Broulik]

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

REVISION SUMMARY
  If we have this feature now, let's use it and give the wallpaper authors the 
credit they deserve rather than being tucked away in the tooltip.

TEST PLAN
  F7871144: Screenshot_20200108_211105.png 


REPOSITORY
  R120 Plasma Workspace

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

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

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

D26506: [KDED KCM] Rewrite as KDeclarative ScrollViewKCM

[Kai Uwe Broulik]

broulik updated this revision to Diff 73089.
broulik added a comment.


  - Filter duplicates

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26506?vs=73085=73089

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

AFFECTED FILES
  CMakeLists.txt
  kcms/kded/CMakeLists.txt
  kcms/kded/filterproxymodel.cpp
  kcms/kded/filterproxymodel.h
  kcms/kded/kcmkded.cpp
  kcms/kded/kcmkded.desktop
  kcms/kded/kcmkded.h
  kcms/kded/modulesmodel.cpp
  kcms/kded/modulesmodel.h
  kcms/kded/package/contents/ui/main.qml
  kcms/kded/package/metadata.desktop

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

D26506: [KDED KCM] Rewrite as KDeclarative ScrollViewKCM

[Kai Uwe Broulik]

broulik updated this revision to Diff 73085.
broulik added a comment.


  - Minor fixes
  - Fix Cmake

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26506?vs=73017=73085

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

AFFECTED FILES
  CMakeLists.txt
  kcms/kded/CMakeLists.txt
  kcms/kded/filterproxymodel.cpp
  kcms/kded/filterproxymodel.h
  kcms/kded/kcmkded.cpp
  kcms/kded/kcmkded.desktop
  kcms/kded/kcmkded.h
  kcms/kded/modulesmodel.cpp
  kcms/kded/modulesmodel.h
  kcms/kded/package/contents/ui/main.qml
  kcms/kded/package/metadata.desktop

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

Re: Plasma 5.18 repo freeze and announce prep

[Kai Uwe Broulik]

In case the link doesn't work (like here), scroll down to the 
"plasma-5_18.md" file in the "ommunity Notes/Plasma" folder...


Am 08.01.20 um 16:09 schrieb Jonathan Riddell:
5.18 repo freeze was a while ago, new repos in 5.18 will be plasma-nano 
and plasma-phone-components.  These two will be marked (in some way) as 
not being LTS.


If you have new features which deserve to be highlighted in the announce 
please add them to this file 
https://share.kde.org/apps/files/?dir=/Community%20Notes/Plasma=1718757


Jonathan

D25449: Start of the new Formats KCM

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> Messages.sh:2
>  #! /usr/bin/env bash
> -$EXTRACTRC *.ui >> rc.cpp
> -$XGETTEXT *.cpp -o $podir/kcmformats.pot
> -rm -f rc.cpp
> -
> +$EXTRACTRC `find . -name "*.qml"` >> rc.qml || exit 11
> +$XGETTEXT `find . -name "*.qml"` -o $podir/kcm5_formats.pot

I don't think this is necessary, at least it's the first time I have ever seen 
this sort of thing

> kcm.cpp:37
> +{
> +qmlRegisterAnonymousType("kcm.kde.org", 1);
> +qmlRegisterAnonymousType("kcm.kde.org", 1);

You're not importing this anywhere, and this still works? Makes me wonder why 
Qt went through the trouble of introducing this when it makes no diference over 
`qmlRegisterType()` ...

Please also use a KCM-specific import name for this, such as 
`org.kde.private.kcms.formats`

> kcm.h:39
> +Formats(QObject* parent, const QVariantList& args);
> +virtual ~Formats() override;
> +

`virtual` isn't necessary here

> kcm.h:42
> +/* creates example strings with the specified Locale. */
> +void updateStringsExample();
> +

Name this `updateExampleStrings` and where is it actually being called from?

> kcm.h:48
> +private:
> +
> +FormatsSettings *m_settings;

Remove whitespace

> kcm_formats.desktop:14
> +Name=Formats
> +Comment=What your kcm is all about?
> +X-KDE-Keywords=Colors, Mouse, Monitor

Yeah, what is it about? :)

> localemodel.cpp:85
> +{
> +Q_UNUSED(parent);
> +return m_data.size();

Ensure that it's not trying to make you a tree model:

  if (parent.isValid()) {
  return 0;
  }
  return m_data.count();

> localemodel.cpp:91
> +{
> +if (!idx.isValid()) {
> +return {};

Use `checkIndex(idx)` which will also ensure it is within bounds iirc

> localemodel.h:1
> +#ifndef LOCALEMODEL_H
> +#define LOCALEMODEL_H

Missing copyright header

> localemodel.h:8
> +#include 
> +#include 
> +#include 

Unused

> localemodel.h:16
> +QString localeValue;
> +};
> +

declare as movable type

> localemodel.h:18
> +
> +class LocaleModel : public QAbstractListModel {
> +Q_OBJECT

`{` on new line

> localemodel.h:21
> +public:
> +enum {Flag = Qt::UserRole + 1, LocaleName, LocaleValue};
> +LocaleModel(QObject *parent = nullptr);

Use `Qt::DisplayRole` for `LocaleName`

Also, what is this ominous "value"? Please give it a clearer name.
Also, coding style.

> localemodel.h:22
> +enum {Flag = Qt::UserRole + 1, LocaleName, LocaleValue};
> +LocaleModel(QObject *parent = nullptr);
> +

`explicit`

> main.qml:38
> +textRole: "localeName"
> +currentIndex: kcm.model.indexFor(kcm.settings.defaultLanguage)
> +

`QAbstractItemModel::match` is usable from QML, then you don't need to roll 
your own `indexFor` method

> main.qml:41
> +/* Kirigami basic list item does not work correctly with 
> ComboBoxex
> +delegate: Kirigami.BasicListItem {
> +icon: model.flag

Have you tried `QQC2.ItemDelegate`?
So annoying QQC to this day can't properly do icons for comboboxes :(

> main.qml:46
> +   */
> +onCurrentIndexChanged: {
> +kcm.settings.defaultLanguage = 
> kcm.model.valueFor(currentIndex)

Use `onActivated`

> main.qml:47
> +onCurrentIndexChanged: {
> +kcm.settings.defaultLanguage = 
> kcm.model.valueFor(currentIndex)
> +}

`QAbstractItemModel::data` is usable from QML:

  var idx = kcm.model.index(currentIndex, 0);
  kcm.settings.defaultLanguage = kcm.model.data(idx, YourModel.LocaleValueRole);

If we're depending on Qt 5.14 there's now also a `valueRole` property you can 
set and then just use `currentValue`

> main.qml:51
> +
> +QQC2.CheckBox {
> +id: detailedSettings

Does this have to be section or something? Iirc FormLayout has a checkable 
section feature

> main.qml:62
> +currentIndex: kcm.model.indexFor(kcm.settings.numeric)
> +onCurrentIndexChanged: {
> +kcm.settings.numeric = kcm.model.valueFor(currentIndex)

Use `onActivated` and everywhere else

REPOSITORY
  R119 Plasma Desktop

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

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

D25375: Start of the accessibility KCM

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> kcmaccess.cpp:141
>  
> -QString result;
> -if ((modifiers & ScrollMask) != 0)
> -if ((modifiers & LockMask) != 0)
> -if ((modifiers & NumMask) != 0)
> -result = i18n("Press %1 while NumLock, CapsLock and 
> ScrollLock are active", keyname);
> -else
> -result = i18n("Press %1 while CapsLock and ScrollLock are 
> active", keyname);
> -else if ((modifiers & NumMask) != 0)
> -result = i18n("Press %1 while NumLock and ScrollLock are 
> active", keyname);
> -else
> -result = i18n("Press %1 while ScrollLock is active", keyname);
> -else if ((modifiers & LockMask) != 0)
> -if ((modifiers & NumMask) != 0)
> -result = i18n("Press %1 while NumLock and CapsLock are active", 
> keyname);
> -else
> -result = i18n("Press %1 while CapsLock is active", keyname);
> -else if ((modifiers & NumMask) != 0)
> -result = i18n("Press %1 while NumLock is active", keyname);
> -else
> -result = i18n("Press %1", keyname);
> -
> -return result;
> +return modifiers & ScrollMask & LockMask & NumMask ? i18n("Press %1 
> while NumLock, CapsLock and ScrollLock are active", keyname)
> + : modifiers & ScrollMask & LockMask ? i18n("Press %1 while CapsLock 
> and ScrollLock are active", keyname)

Are you sure this shouldn't be something like

  modifiers & (ScrollMask | LockMask | NumMask)

> kcmaccess.cpp:170
>  
> -ui.setupUi(this);
> -
> -connect(ui.soundButton, ::clicked, this, 
> ::selectSound);
> -connect(ui.customBell, ::clicked, this, 
> ::checkAccess);
> -connect(ui.systemBell, ::clicked, this, 
> ::configChanged);
> -connect(ui.customBell, ::clicked, this, 
> ::configChanged);
> -connect(ui.soundEdit, ::textChanged, this, 
> ::configChanged);
> -
> -connect(ui.invertScreen, ::clicked, this, 
> ::configChanged);
> -connect(ui.flashScreen, ::clicked, this, 
> ::configChanged);
> -connect(ui.visibleBell, ::clicked, this, 
> ::configChanged);
> -connect(ui.visibleBell, ::clicked, this, 
> ::checkAccess);
> -connect(ui.colorButton, ::clicked, this, 
> ::changeFlashScreenColor);
> -
> -connect(ui.invertScreen, ::clicked, this, 
> ::invertClicked);
> -connect(ui.flashScreen, ::clicked, this, 
> ::flashClicked);
> -
> -connect(ui.duration, static_cast (QSpinBox::*)(int)>(::valueChanged), this, 
> ::configChanged);
> -
> -
> -// modifier key settings ---
> -
> -connect(ui.stickyKeys, ::clicked, this, 
> ::configChanged);
> -connect(ui.stickyKeysLock, ::clicked, this, 
> ::configChanged);
> -connect(ui.stickyKeysAutoOff, ::clicked, this, 
> ::configChanged);
> -connect(ui.stickyKeys, ::clicked, this, 
> ::checkAccess);
> -
> -connect(ui.stickyKeysBeep, ::clicked, this, 
> ::configChanged);
> -connect(ui.toggleKeysBeep, ::clicked, this, 
> ::configChanged);
> -connect(ui.kNotifyModifiers, ::clicked, this, 
> ::configChanged);
> -connect(ui.kNotifyModifiers, ::clicked, this, 
> ::checkAccess);
> -connect(ui.kNotifyModifiersButton, ::clicked, this, 
> ::configureKNotify);
> -
> -// key filter settings -
> -
> -connect(ui.slowKeysDelay, static_cast (QSpinBox::*)(int)>(::valueChanged), this, 
> ::configChanged);
> -connect(ui.slowKeys, ::clicked, this, 
> ::configChanged);
> -connect(ui.slowKeys, ::clicked, this, 
> ::checkAccess);
> -
> -connect(ui.slowKeysPressBeep, ::clicked, this, 
> ::configChanged);
> -connect(ui.slowKeysAcceptBeep, ::clicked, this, 
> ::configChanged);
> -connect(ui.slowKeysRejectBeep, ::clicked, this, 
> ::configChanged);
> -
> -connect(ui.bounceKeysDelay, static_cast (QSpinBox::*)(int)>(::valueChanged), this, 
> ::configChanged);
> -connect(ui.bounceKeys, ::clicked, this, 
> ::configChanged);
> -connect(ui.bounceKeysRejectBeep, ::clicked, this, 
> ::configChanged);
> -connect(ui.bounceKeys, ::clicked, this, 
> ::checkAccess);
> -
> -// gestures 
> -
> -QString shortcut = mouseKeysShortcut(QX11Info::display());
> -if (shortcut.isEmpty())
> -ui.gestures->setToolTip(i18n("Here you can activate keyboard 
> gestures that turn on the following features: \n"
> -"Sticky keys: Press Shift key 5 
> consecutive times\n"
> -"Slow keys: Hold down Shift for 8 
> seconds"));
> -else
> -ui.gestures->setToolTip(i18n("Here you can activate keyboard 
> gestures that turn on the following features: \n"
> -"Mouse Keys: %1\n"
> -"Sticky keys: Press Shift key 5 
> consecutive times\n"
> -"Slow keys: Hold down Shift for 8 
> seconds", shortcut));
> -
> -

D26516: [Icon Item] Check URL validity and scheme

[Kai Uwe Broulik]

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

REVISION SUMMARY
  Creating a `KFileItemList` with a `KFileItem` with a URL that doesn't have a 
scheme triggers an assert in `KFileItemListProperties`.
  It doesn't really make sense to have anyway.

TEST PLAN
  - Created an icon item with a desktop file with a `URL=foo`, no longer quit 
plasmashell when right clicking.
  
  Instead, the menu is empty now. Open with actions for proper files still work

REPOSITORY
  R120 Plasma Workspace

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

AFFECTED FILES
  applets/icon/iconapplet.cpp

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

D26506: [KDED KCM] Rewrite as KDeclarative ScrollViewKCM

[Kai Uwe Broulik]

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

REVISION SUMMARY
  This rewrites the "Background services" KCM in QML using `ScrollViewKCM`.
  The two separate list views are merged into a single one with the 
configurable services at the top, and the ones that are loaded on-demand and 
"only for your convenience" at the bottom.
  A search field is added searching through name and plugin ID. Since the 
sortable table headers are gone, a filter combo is provided instead to filter 
for all, running, or non running services.
  As an extra Schmankerl when starting a service that immediately disables 
itself again (which technically isn't an error that would be indicated as such) 
a hint is shown to the user so they're not left wondering why it doesn't start.
  Furthermore, the code is cleaned up a lot (quite eerie, adding a 2020 
Copyright to an existing 2002 one :), ported to json plugin data, and a proper 
`QAbstractListModel` added.

TEST PLAN
  F7868523: Screenshot_20200107_223707.png 

  F7868524: Screenshot_20200107_223814.png 

  Trying to start the device automounter which disables itself on load when 
automounting is disabled in the KCM
  F7868525: Screenshot_20200107_223859.png 

  Starting or stopping a service shows a little animation
  F7868531: Screenshot_20200107_224027.png 

  This is mostly for when you apply changes and kded reloads, it will start all 
autoloaded modules, even if user manually stopped them. Originally I wanted to 
show an inline message along the lines of "some services were started again 
when you saved your changes because..." but that turned out to be too 
brittle/unreliable.
  
  Issues remaining:
  
  - Somehow that Kirigami listdelegate feature of adding right padding to take 
into account the scrollbar doesn't work in ScrollViewKCM
  - Since it's using delegaterecycler, the animation will play as you scroll up 
and down since as far as the item is concerned, status does change.
  - Some qt 5.14 adjustments (qregisteranonymoustype)
  - I get a binding loop on `Kirigami.AbstractListItem.implicitHeight` for my 
delegate for some reason

REPOSITORY
  R119 Plasma Desktop

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

AFFECTED FILES
  kcms/kded/CMakeLists.txt
  kcms/kded/filterproxymodel.cpp
  kcms/kded/filterproxymodel.h
  kcms/kded/kcmkded.cpp
  kcms/kded/kcmkded.desktop
  kcms/kded/kcmkded.h
  kcms/kded/modulesmodel.cpp
  kcms/kded/modulesmodel.h
  kcms/kded/package/contents/ui/main.qml
  kcms/kded/package/metadata.desktop

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

D26489: Use qmlRegisterAnonymousType

[Kai Uwe Broulik]

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


  needs a qt version `ifdef`

REPOSITORY
  R112 Milou

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

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

D26485: [Pager] Fix switching pages on

[Kai Uwe Broulik]

broulik closed this revision.
broulik added a comment.


  f5d1675a0dc1a1a0098eb5b1c727b5fe197e9930 


REPOSITORY
  R119 Plasma Desktop

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

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

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

[Kai Uwe Broulik]

This revision was automatically updated to reflect the committed changes.
Closed by commit R119:d2d30a9667d2: [KRunner KCM] Mark KCM as dirty when plugin 
configuration changes (authored by broulik).

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26445?vs=72830=72957

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

AFFECTED FILES
  kcms/runners/kcm.cpp

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

D26425: [Notifications KCM] Fixup current item syncing logic

[Kai Uwe Broulik]

This revision was automatically updated to reflect the committed changes.
Closed by commit R119:b639338291d4: [Notifications KCM] Fixup current item 
syncing logic (authored by broulik).

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26425?vs=72770=72958

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

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

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

D26416: Kicker/Documents optimization: Lazily build KFileItem

[Kai Uwe Broulik]

broulik added a comment.


  Would be lovely in the future to cache those items and make it a proper model 
with backing data rather than requesting everything on demand every time `data` 
is called.

REPOSITORY
  R120 Plasma Workspace

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

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

D26483: Add /usr/share/wallpapers as a default wallpaper slideshow location

[Kai Uwe Broulik]

broulik added a comment.


  Perhaps we could leverage some code from Task Manager to have a fake 
`preferred://wallpaperlocation` URL or something like that?

REPOSITORY
  R120 Plasma Workspace

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

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

D26485: [Pager] Fix switching pages on

[Kai Uwe Broulik]

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

REVISION SUMMARY
  The code was clearly intending to use the index, as indicated by a comparison 
with `currentPage` (which is an `int`) and `changePage` taking an `int`.
  
  BUG: 415423
  FIXED-IN: 5.17.5

TEST PLAN
  - Dragged a file onto pager, waited for desktop switch, plasma no longer 
crashed
  - Activity pager also works fine the same way

REPOSITORY
  R119 Plasma Desktop

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

AFFECTED FILES
  applets/pager/package/contents/ui/main.qml

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

D26422: [Notifications] Fix ListView attached property

[Kai Uwe Broulik]

This revision was automatically updated to reflect the committed changes.
Closed by commit R120:cc9a1a6803d2: [Notifications] Fix ListView attached 
property (authored by broulik).

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26422?vs=72768=72948

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

AFFECTED FILES
  applets/notifications/package/contents/ui/FullRepresentation.qml

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

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

[Kai Uwe Broulik]

broulik added a comment.


  > why this is a `count`
  
  It's only consistent with how we usually do it elsewhere, and I think for a 
list model having a `count` in general makes more sense/is more generic/more 
useful than an `isEmpty` property

REPOSITORY
  R845 Plasma Vault

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

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

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

[Kai Uwe Broulik]

broulik added a comment.


  +1

INLINE COMMENTS

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

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

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

Coding style

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

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

REPOSITORY
  R845 Plasma Vault

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

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

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

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

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

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

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

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

REPOSITORY
  R845 Plasma Vault

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

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

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

[Kai Uwe Broulik]

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

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

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

REPOSITORY
  R119 Plasma Desktop

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

AFFECTED FILES
  kcms/runners/kcm.cpp

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