D17210: Added proxy and user settings

[Jan Grulich]

jgrulich added inline comments.

INLINE COMMENTS

> proxysetting.cpp:25
> +#include 
> +
> +NetworkManager::ProxySettingPrivate::ProxySettingPrivate()

It looks that the proxy setting has been introduced in NetworkManager 1.6. This 
means that for all property defines, you have to add ifdef the same way you did 
for ip tunnel setting.

> proxysetting.h:35
> +/**
> + * Represents generic setting
> + */

Represents proxy setting

> proxysetting.h:52
> +
> +void setMethod(quint32 method);
> +quint32 method() const;

This can be turned into an enum.

Possible values are:
NM_SETTING_PROXY_METHOD_NONE = 0,
NM_SETTING_PROXY_METHOD_AUTO = 1,

So I would do:
enum Mode { None = NM_SETTING_PROXY_METHOD_NONE, auto = 
NM_SETTING_PROXY_METHOD_AUTO }

> usersetting.cpp:25
> +#include 
> +
> +NetworkManager::UserSettingPrivate::UserSettingPrivate()

User setting was introduced with NM 1.8. Here should be ifdef to check the 
version and define NM_SETTING_USER_DATA in case the version is older.

> usersetting.cpp:27
> +NetworkManager::UserSettingPrivate::UserSettingPrivate()
> +: name(NM_SETTING_IP_TUNNEL_SETTING_NAME)
> +{ }

Should be NM_SETTING_USER_SETTING_NAME

> usersetting.h:35
> +/**
> + * Represents generic setting
> + */

Represents user setting

> usersetting.h:52
> +NMStringMap data() const;
> +
> +

No reason for such a big space.

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

To: pranavgade, jgrulich
Cc: ngraham, kde-frameworks-devel, michaelh, bruns

D17234: do not attempt to link phononexperimental :O

[Harald Sitter]

sitter retitled this revision from "do not attempt to link phononexperimental 
:anguished:" to "do not attempt to link phononexperimental :O".

REPOSITORY
  R305 KNotifyConfig

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

To: sitter, broulik
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17234: do not attempt to link phononexperimental :O

[Harald Sitter]

sitter edited the test plan for this revision.

REPOSITORY
  R305 KNotifyConfig

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

To: sitter, broulik
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17234: do not attempt to link phononexperimental :anguished:

[Harald Sitter]

sitter created this revision.
sitter added a reviewer: broulik.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
sitter requested review of this revision.

REVISION SUMMARY
  ${PHONON_LIBS} as the name suggests links all libraries, that includes
  the experimental library. it's not even used though.
  move to imported target to avoid linking experimental and also get rid
  of the include_directories call

TEST PLAN
➜ DESTDIR=x ninja install &> /dev/null; grep -ri phonon4qt5experimental x

x/usr/lib/x86_64-linux-gnu/cmake/KF5NotifyConfig/KF5NotifyConfigTargets-debug.cmake:
  IMPORTED_LINK_DEPENDENT_LIBRARIES_DEBUG 
"KF5::I18n;KF5::KIOWidgets;Phonon::phonon4qt5;Phonon::phonon4qt5experimental;Qt5::DBus"
  
  with new code
  
`
➜ DESTDIR=x ninja install &> /dev/null; grep -ri phonon4qt5experimental x

REPOSITORY
  R305 KNotifyConfig

BRANCH
  master

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

AFFECTED FILES
  src/CMakeLists.txt

To: sitter, broulik
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17234: do not attempt to link phononexperimental :O

[Aleix Pol Gonzalez]

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

REPOSITORY
  R305 KNotifyConfig

BRANCH
  master

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

To: sitter, broulik, apol
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17234: do not attempt to link phononexperimental :O

[Harald Sitter]

This revision was automatically updated to reflect the committed changes.
Closed by commit R305:f6d55baf5aa8: do not attempt to link phononexperimental 
:O (authored by sitter).

REPOSITORY
  R305 KNotifyConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17234?vs=46460&id=46461

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

AFFECTED FILES
  src/CMakeLists.txt

To: sitter, broulik, apol
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D16249: Warn user before copy/move job if the file size exceeds the maximum possible file size in FAT32 filesystem(4 GB)

[Stefan Brüns]

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

REPOSITORY
  R241 KIO

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

To: shubham, ngraham, #frameworks, bruns, dfaure
Cc: elvisangelaccio, dfaure, cfeck, bruns, kde-frameworks-devel, michaelh, 
ngraham

D16249: Warn user before copy/move job if the file size exceeds the maximum possible file size in FAT32 filesystem(4 GB)

[David Edmundson]

davidedmundson added a comment.


  Also +1 from me.
  
  However, I would strongly suggest waiting till after Saturday when frameworks 
is tagged. 
  Then you'll have a month for translators and for some testing.

REPOSITORY
  R241 KIO

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

To: shubham, ngraham, #frameworks, bruns, dfaure
Cc: davidedmundson, elvisangelaccio, dfaure, cfeck, bruns, 
kde-frameworks-devel, michaelh, ngraham

KDE CI: Frameworks » plasma-framework » kf5-qt5 SUSEQt5.10 - Build # 272 - Still Unstable!

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks/job/plasma-framework/job/kf5-qt5%20SUSEQt5.10/272/
 Project:
kf5-qt5 SUSEQt5.10
 Date of build:
Thu, 29 Nov 2018 12:58:37 +
 Build duration:
6 min 51 sec and counting
   BUILD ARTIFACTS
  compat_reports/KF5Plasma_compat_report.html
   JUnit Tests
  Name: (root) Failed: 6 test(s), Passed: 9 test(s), Skipped: 0 test(s), Total: 15 test(s)Failed: TestSuite.dialognativetestFailed: TestSuite.plasma-configmodeltestFailed: TestSuite.plasma-fallbackpackagetestFailed: TestSuite.plasma-iconitemtestFailed: TestSuite.plasma-packagestructuretestFailed: TestSuite.plasma-storagetest
   Cobertura Report
  
   Project Coverage Summary
  
   Name
  PackagesFilesClassesLinesConditionalsCobertura Coverage Report33%
(6/18)36%
(45/126)36%
(45/126)27%
(3608/13324)18%
(1817/9848)Coverage Breakdown by Package
Name
   FilesClassesLinesConditionalsautotests86%
(12/14)86%
(12/14)55%
(612/1117)29%
(315/1086)src.declarativeimports.calendar0%
(0/6)0%
(0/6)0%
(0/464)0%
(0/243)src.declarativeimports.core31%
(5/16)31%
(5/16)13%
(299/2248)7%
(96/1456)src.declarativeimports.plasmacomponents0%
(0/6)0%
(0/6)0%
(0/518)0%
(0/207)src.declarativeimports.plasmaextracomponents0%
(0/3)0%
(0/3)0%
(0/42)0%
(0/22)src.declarativeimports.platformcomponents0%
(0/3)0%
(0/3)0%
(0/58)0%
(0/14)src.declarativeimports.platformcomponents.utils0%
(0/2)0%
(0/2)0%
(0/14)0%
(0/2)src.plasma64%
(14/22)64%
(14/22)40%
(1410/3491)28%
(787/2817)src.plasma.packagestructure0%
(0/7)0%
(0/7)0%
(0/134)0%
(0/12)src.plasma.private50%
(9/18)50%
(9/18)43%
(674/1574)29%
(301/1034)src.plasma.scripting0%
(0/3)0%
(0/3)0%
(0/162)0%
(0/128)src.plasmapkg0%
(0/1)0%
(0/1)0%
(0/45)0%
(0/40)src.plasmaquick33%
(4/12)33%
(4/12)29%
(582/2018)18%
(313/1721)src.plasmaquick.private50%
(1/2)50%
(1/2)29%
(31/106)36%
(5/14)src.scriptengines.qml.plasmoid0%
(0/6)0%
(0/6)0%
(0/1178)0%
(0/1028)tests.dpi0%
(0/2)0%
(0/2)0%
(0/21)0%
(0/2)tests.kplugins0%

D17190: Add level api from Kirigami.Heading

[Marco Martin]

This revision was automatically updated to reflect the committed changes.
Closed by commit R236:d3964629cbff: Add level api from Kirigami.Heading 
(authored by mart).

REPOSITORY
  R236 KWidgetsAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17190?vs=46322&id=46463

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

AFFECTED FILES
  src/ktitlewidget.cpp
  src/ktitlewidget.h

To: mart, #plasma, #vdg, ngraham
Cc: ngraham, kde-frameworks-devel, michaelh, bruns

D17191: Perfect alignment between QML and QWidget KCM titles

[Marco Martin]

This revision was automatically updated to reflect the committed changes.
Closed by commit R295:bba3150027f3: Perfect alignment between QML and QWidget 
KCM titles (authored by mart).

REPOSITORY
  R295 KCMUtils

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17191?vs=46323&id=46464

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

AFFECTED FILES
  src/kcmoduleqml.cpp

To: mart, #plasma, #vdg, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17210: Added proxy and user settings

[Pranav Gade]

pranavgade updated this revision to Diff 46467.
pranavgade marked 6 inline comments as done.
pranavgade added a comment.


  Updated code as required, 
  fixed a minor error in iptunnelsettings

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17210?vs=46444&id=46467

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

AFFECTED FILES
  src/settings/iptunnelsetting.cpp
  src/settings/proxysetting.cpp
  src/settings/proxysetting.h
  src/settings/proxysetting_p.h
  src/settings/usersetting.cpp
  src/settings/usersetting.h

To: pranavgade, jgrulich
Cc: ngraham, kde-frameworks-devel, michaelh, bruns

D17210: Added proxy and user settings

[Jan Grulich]

jgrulich added inline comments.

INLINE COMMENTS

> iptunnelsetting.cpp:35
>  , encapsulationLimit(0)
> -, flags(IpTunnelSetting::Unknown)
> +, flags(IpTunnelSetting::None)
>  , flowLabel(0)

This is an unrelated change, submit it in a different review, but thanks for 
spotting this.

> proxysetting.cpp:26
>  
> +#if NM_CHECK_VERSION(1, 16, 0)
> +#define NM_SETTING_PROXY_SETTING_NAME"proxy"

I said NM 1.6, not 1.16 and you need !NM_CHECK_VERSION(1, 6, 0).

> proxysetting.h:42
>  typedef QList List;
> +enum Mode
> +{

Coding style. It should be:

enum Mode {

}

> usersetting.cpp:26
>  
> +#if NM_CHECK_VERSION(1, 8, 0)
> +#define NM_SETTING_USER_SETTING_NAME"user"

Same here. It should be !NM_CHECK_VERSION(1, 8, 0).

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

To: pranavgade, jgrulich
Cc: ngraham, kde-frameworks-devel, michaelh, bruns

D17210: Added proxy and user settings

[Pranav Gade]

pranavgade updated this revision to Diff 46469.
pranavgade added a comment.


  made some changes

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17210?vs=46467&id=46469

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

AFFECTED FILES
  autotests/settings/CMakeLists.txt
  autotests/settings/proxysettingtest.cpp
  autotests/settings/proxysettingtest.h
  autotests/settings/usersettingtest.cpp
  autotests/settings/usersettingtest.h
  src/CMakeLists.txt
  src/settings/proxysetting.cpp
  src/settings/proxysetting.h
  src/settings/proxysetting_p.h
  src/settings/setting.cpp
  src/settings/setting.h
  src/settings/usersetting.cpp
  src/settings/usersetting.h
  src/settings/usersetting_p.h

To: pranavgade, jgrulich
Cc: ngraham, kde-frameworks-devel, michaelh, bruns

D17193: KateViewInternal: Remove unneeded functions

[loh tar]

loh.tar updated this revision to Diff 46473.
loh.tar edited the summary of this revision.
loh.tar edited the test plan for this revision.
loh.tar added a comment.


  - Prefer function calls for member excess
  - Merge KateViewInternal::updateView with KateViewInternal::doUpdateView
  - Add documentation to related functions and some near by cosmetic. Well, 
"Better no docu than bad docu", so if that is to difficult to check now, I can 
remove it and ship as own diff
  - Complete commit message
  - Remove Testplan, I'm always surpsised to see that later in the commit 
message

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17193?vs=46411&id=46473

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

AFFECTED FILES
  src/view/kateview.cpp
  src/view/kateview.h
  src/view/kateviewinternal.cpp
  src/view/kateviewinternal.h

To: loh.tar, #ktexteditor, cullmann
Cc: cullmann, kwrite-devel, kde-frameworks-devel, #ktexteditor, hase, michaelh, 
ngraham, bruns, demsking, sars, dhaumann

D17210: Added proxy and user settings

[Pranav Gade]

pranavgade marked 3 inline comments as done.
pranavgade added inline comments.

INLINE COMMENTS

> jgrulich wrote in proxysetting.cpp:25
> It looks that the proxy setting has been introduced in NetworkManager 1.6. 
> This means that for all property defines, you have to add ifdef the same way 
> you did for ip tunnel setting.

Fixed according to:
https://developer.gnome.org/libnm/stable/NMSettingProxy.html#NM-SETTING-PROXY-METHOD:CAPS

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

To: pranavgade, jgrulich
Cc: ngraham, kde-frameworks-devel, michaelh, bruns

D17210: Added proxy and user settings

[Pranav Gade]

pranavgade marked 3 inline comments as done.

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

To: pranavgade, jgrulich
Cc: ngraham, kde-frameworks-devel, michaelh, bruns

D17210: Added proxy and user settings

[Christoph Feck]

cfeck added a comment.


  Could you please set the Repository field?

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

To: pranavgade, jgrulich
Cc: cfeck, ngraham, kde-frameworks-devel, michaelh, bruns

D17210: Added proxy and user settings

[Pranav Gade]

pranavgade set the repository for this revision to R282 NetworkManagerQt.

REPOSITORY
  R282 NetworkManagerQt

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

To: pranavgade, jgrulich
Cc: cfeck, ngraham, kde-frameworks-devel, michaelh, bruns

D17193: KateViewInternal: Remove unneeded functions

[Christoph Cullmann]

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


  I am happy with this ;=)
  Thanks for adding documentation to the header, such stuff is very welcome!

REPOSITORY
  R39 KTextEditor

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

To: loh.tar, #ktexteditor, cullmann
Cc: cullmann, kwrite-devel, kde-frameworks-devel, #ktexteditor, hase, michaelh, 
ngraham, bruns, demsking, sars, dhaumann

D17193: KateViewInternal: Remove unneeded functions

[Christoph Cullmann]

This revision was automatically updated to reflect the committed changes.
Closed by commit R39:8b4cd5f37947: KateViewInternal: Remove unneeded functions 
(authored by loh.tar, committed by cullmann).

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17193?vs=46473&id=46478

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

AFFECTED FILES
  src/view/kateview.cpp
  src/view/kateview.h
  src/view/kateviewinternal.cpp
  src/view/kateviewinternal.h

To: loh.tar, #ktexteditor, cullmann
Cc: cullmann, kwrite-devel, kde-frameworks-devel, #ktexteditor, hase, michaelh, 
ngraham, bruns, demsking, sars, dhaumann

D17220: Use red X in disabled and muted status icons consistently

[TrickyRicky]

trickyricky26 added a comment.


  I think the distinction is useful. The off icons would then look like this:
  F6446567: microphone-sensitivity-high-22.svg.png 

  
  F6446566: audio-volume-high-22.svg.png 
  
  Also, I've experimented with a circle with one line through it, as that 
represents muted better than an X which looks too much like an error:
  F6446578: audio-volume-muted-alt-22.svg.png 


REPOSITORY
  R266 Breeze Icons

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

To: trickyricky26, #vdg, ndavis
Cc: ndavis, ngraham, kde-frameworks-devel, michaelh, bruns

D17220: Use red X in disabled and muted status icons consistently

[Andres Betts]

abetts added a comment.


  In D17220#368147 , @trickyricky26 
wrote:
  
  > I think the distinction is useful. The off icons would then look like this:
  >  F6446567: microphone-sensitivity-high-22.svg.png 

  >
  > F6446566: audio-volume-high-22.svg.png 

  >  Should this style then also be applied to other "off" status icons like 
touchpad and camera?
  >
  > Also, I've experimented with a circle with one line through it, as that 
represents muted better than an X which looks too much like an error:
  >  F6446578: audio-volume-muted-alt-22.svg.png 

  
  
  These look really good. My only warning to the team is that we don't add too 
many elements to the icon that will make it expand in area. We should strive to 
keeping size constraints so that we don't introduce too many elements into the 
icon that will make it be confusing visually.

REPOSITORY
  R266 Breeze Icons

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

To: trickyricky26, #vdg, ndavis
Cc: abetts, ndavis, ngraham, kde-frameworks-devel, michaelh, bruns

D17220: Use red X in disabled and muted status icons consistently

[Nathaniel Graham]

ngraham added a comment.


  In D17220#368147 , @trickyricky26 
wrote:
  
  > F6446578: audio-volume-muted-alt-22.svg.png 

  
  
  This is really nice, at least at large size. How does it fare in the UI when 
it's tiny? Is the red part still distinguishable?

REPOSITORY
  R266 Breeze Icons

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

To: trickyricky26, #vdg, ndavis
Cc: abetts, ndavis, ngraham, kde-frameworks-devel, michaelh, bruns

D17220: Use red X in disabled and muted status icons consistently

[TrickyRicky]

trickyricky26 added a comment.


  `22px` is okay imo; however I've enlarged it quite a bit for `16px`:
  F6446668: audio-volume-muted-alt-16.svg.png 


REPOSITORY
  R266 Breeze Icons

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

To: trickyricky26, #vdg, ndavis
Cc: abetts, ndavis, ngraham, kde-frameworks-devel, michaelh, bruns

D17210: Added proxy and user settings

[Jan Grulich]

jgrulich added inline comments.

INLINE COMMENTS

> proxysetting.cpp:37
> +: name(NM_SETTING_PROXY_SETTING_NAME)
> +, browserOnly(true)
> +, method(ProxySetting::None)

Default value is false.

> usersetting.cpp:94
> +}
> +
> +

One empty line is enough.

REPOSITORY
  R282 NetworkManagerQt

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

To: pranavgade, jgrulich
Cc: cfeck, ngraham, kde-frameworks-devel, michaelh, bruns

D17238: Explicitly create QDateTime with UTC time

[Kai Uwe Broulik]

broulik created this revision.
broulik added reviewers: dfaure, davidedmundson.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
broulik requested review of this revision.

REVISION SUMMARY
  Otherwise it spends half of the time doing timezone conversions to local time.

TEST PLAN
  - Copied a million files. Time spent in `QDateTime::setMSecsSinceEpoch` went 
from 44% to 0.1%, total time of `addCopyInfoFromUDSEnty` went from 57% to 36%, 
now the bottleneck is all that path concatenation causing URL decoding.
  - Verified that copied files still have correct mtime
  - Verified that "overwrite?" dialog still shows correct mtime

REPOSITORY
  R241 KIO

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

AFFECTED FILES
  src/core/copyjob.cpp

To: broulik, dfaure, davidedmundson
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

kde-frameworks-devel@kde.org

[loh tar]

loh.tar created this revision.
loh.tar added reviewers: cullmann, KTextEditor.
Herald added projects: Kate, Frameworks.
Herald added subscribers: kde-frameworks-devel, kwrite-devel.
loh.tar requested review of this revision.

REVISION SUMMARY
  - 'm_doc->' => 'doc()->'
  - 'm_viewInternal->m_cursor' => 'cursorPosition()'

REPOSITORY
  R39 KTextEditor

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

AFFECTED FILES
  src/view/kateview.cpp

To: loh.tar, cullmann, #ktexteditor
Cc: kwrite-devel, kde-frameworks-devel, #ktexteditor, hase, michaelh, ngraham, 
bruns, demsking, cullmann, sars, dhaumann

kde-frameworks-devel@kde.org

[loh tar]

loh.tar added a comment.


  I can split it if you like in two diff
  
  - 'm_doc->' => 'doc()->' ~100 hits, perhaps not the best idea? A search for 
doc() has 24 hits (without this patch)
  - 'm_viewInternal->m_cursor' => 'cursorPosition()' ~10 hits
  
  Next would be look for inline candidates, look for S&R in KateViewInternal

REPOSITORY
  R39 KTextEditor

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

To: loh.tar, cullmann, #ktexteditor
Cc: kwrite-devel, kde-frameworks-devel, #ktexteditor, hase, michaelh, ngraham, 
bruns, demsking, cullmann, sars, dhaumann

D17210: Added proxy and user settings

[Pranav Gade]

pranavgade updated this revision to Diff 46488.
pranavgade marked 2 inline comments as done.

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17210?vs=46469&id=46488

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

AFFECTED FILES
  autotests/settings/CMakeLists.txt
  autotests/settings/proxysettingtest.cpp
  autotests/settings/proxysettingtest.h
  autotests/settings/usersettingtest.cpp
  autotests/settings/usersettingtest.h
  src/CMakeLists.txt
  src/settings/proxysetting.cpp
  src/settings/proxysetting.h
  src/settings/proxysetting_p.h
  src/settings/setting.cpp
  src/settings/setting.h
  src/settings/usersetting.cpp
  src/settings/usersetting.h
  src/settings/usersetting_p.h

To: pranavgade, jgrulich
Cc: cfeck, ngraham, kde-frameworks-devel, michaelh, bruns

D17210: Added proxy and user settings

[Jan Grulich]

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

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

To: pranavgade, jgrulich
Cc: cfeck, ngraham, kde-frameworks-devel, michaelh, bruns

D16249: Warn user before copy/move job if the file size exceeds the maximum possible file size in FAT32 filesystem(4 GB)

[Nathaniel Graham]

ngraham added a comment.


  I agree. Because this contains new strings and we're so close to the 5.53 
release, let's land this for Frameworks 5.54 after tagging.
  
  @shubham I'm afraid this means you'll now need to change all the instances of 
`5.53` to `5.54`! 😛 I know it's taken a long time, but don't worry, it will be 
landed soon...

REPOSITORY
  R241 KIO

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

To: shubham, ngraham, #frameworks, bruns, dfaure
Cc: davidedmundson, elvisangelaccio, dfaure, cfeck, bruns, 
kde-frameworks-devel, michaelh, ngraham

D17238: Explicitly create QDateTime with UTC time

[David Faure]

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


  Yep, tzset is really slow. I suppose there are other similar lines of code in 
KIO that would benefit from the same fix? KFileItem for example?

REPOSITORY
  R241 KIO

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

To: broulik, dfaure, davidedmundson
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17238: Explicitly create QDateTime with UTC time

[Kai Uwe Broulik]

This revision was automatically updated to reflect the committed changes.
Closed by commit R241:a9592e0b69c9: Explicitly create QDateTime with UTC time 
(authored by broulik).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17238?vs=46485&id=46489

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

AFFECTED FILES
  src/core/copyjob.cpp

To: broulik, dfaure, davidedmundson
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17239: ovs-bridge and ovs-interface setting

[Pranav Gade]

pranavgade created this revision.
pranavgade added a reviewer: jgrulich.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
pranavgade requested review of this revision.

REVISION SUMMARY
  Added ovs-bridge and ovs-interface setting according to:
  https://developer.gnome.org/NetworkManager/stable/settings-ovs-bridge.html 
https://developer.gnome.org/NetworkManager/stable/settings-ovs-interface.html

REPOSITORY
  R282 NetworkManagerQt

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

AFFECTED FILES
  autotests/settings/CMakeLists.txt
  autotests/settings/ovsbridgesettingtest.cpp
  autotests/settings/ovsbridgesettingtest.h
  autotests/settings/ovsinterfacesettingtest.cpp
  autotests/settings/ovsinterfacesettingtest.h
  src/CMakeLists.txt
  src/settings/ovsbridgesetting.cpp
  src/settings/ovsbridgesetting.h
  src/settings/ovsbridgesetting_p.h
  src/settings/ovsinterfacesetting.cpp
  src/settings/ovsinterfacesetting.h
  src/settings/ovsinterfacesetting_p.h
  src/settings/setting.cpp
  src/settings/setting.h

To: pranavgade, jgrulich
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D16249: Warn user before copy/move job if the file size exceeds the maximum possible file size in FAT32 filesystem(4 GB)

[Shubham]

shubham updated this revision to Diff 46490.
shubham added a comment.


  Change to KF 5.54
  Will land it after saturday. This patch actually missed 2 tagging (5.52 and 
5.53)

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16249?vs=46235&id=46490

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

AFFECTED FILES
  src/core/copyjob.cpp
  src/core/global.h
  src/core/job_error.cpp

To: shubham, ngraham, #frameworks, bruns, dfaure
Cc: davidedmundson, elvisangelaccio, dfaure, cfeck, bruns, 
kde-frameworks-devel, michaelh, ngraham

D16249: Warn user before copy/move job if the file size exceeds the maximum possible file size in FAT32 filesystem(4 GB)

[Nathaniel Graham]

ngraham added a comment.


  Awesome, can't wait to have this in!

REPOSITORY
  R241 KIO

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

To: shubham, ngraham, #frameworks, bruns, dfaure
Cc: davidedmundson, elvisangelaccio, dfaure, cfeck, bruns, 
kde-frameworks-devel, michaelh, ngraham

D17241: Disable highlighting for lines longer than 1024 characters.

[Kåre Särs]

sars created this revision.
Herald added projects: Kate, Frameworks.
Herald added subscribers: kde-frameworks-devel, kwrite-devel.
sars requested review of this revision.

REPOSITORY
  R39 KTextEditor

BRANCH
  master

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

AFFECTED FILES
  src/render/katerenderer.cpp

To: sars
Cc: kwrite-devel, kde-frameworks-devel, hase, michaelh, ngraham, bruns, 
demsking, cullmann, sars, dhaumann

D17241: Disable highlighting for lines longer than 1024 characters.

[Shubham]

shubham added reviewers: cullmann, vkrause.

REPOSITORY
  R39 KTextEditor

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

To: sars, cullmann, vkrause
Cc: kwrite-devel, kde-frameworks-devel, hase, michaelh, ngraham, bruns, 
demsking, cullmann, sars, dhaumann

D17241: Disable highlighting for lines longer than 1024 characters.

[Sven Brauch]

brauch added a comment.


  Shouldn't this change simultaneously remove the line length limit ...?

REPOSITORY
  R39 KTextEditor

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

To: sars, cullmann, vkrause
Cc: brauch, kwrite-devel, kde-frameworks-devel, hase, michaelh, ngraham, bruns, 
demsking, cullmann, sars, dhaumann

D17241: WIP: Disable highlighting for lines longer than 1024 characters.

[Kåre Särs]

sars retitled this revision from "Disable highlighting for lines longer than 
1024 characters." to "WIP: Disable highlighting for lines longer than 1024 
characters.".
sars edited the summary of this revision.

REPOSITORY
  R39 KTextEditor

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

To: sars, cullmann, vkrause
Cc: brauch, kwrite-devel, kde-frameworks-devel, hase, michaelh, ngraham, bruns, 
demsking, cullmann, sars, dhaumann

D17243: Only cal updateView() in visibleRange() when endPos() is invalid.

[Kåre Särs]

sars created this revision.
sars added reviewers: cullmann, Kate, dhaumann.
sars added a project: Kate.
Herald added subscribers: kde-frameworks-devel, kwrite-devel.
Herald added a project: Frameworks.
sars requested review of this revision.

REVISION SUMMARY
  visibleRange() has a side-effect that the visible range is updated every time 
the function is called. This is a big problem in createHighlights().
  
  I created a dummy XML file that has an almost 4096 characters long line. I 
then used the search plugin to replace >< with >\n< to split the line.
  
  Without the patch on my computer it took 1 min 30s to split the lines.
  With this patch the time went down to 48s.
  
  The time is still bad, but much better ;)
  
  I wonder if the comment on line 3318 "//ensure that the view is up-to-date, 
otherwise 'endPos()' might fail!" warns about some corner case that I have not 
encountered...

REPOSITORY
  R39 KTextEditor

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

AFFECTED FILES
  src/view/kateview.cpp

To: sars, cullmann, #kate, dhaumann
Cc: kwrite-devel, kde-frameworks-devel, hase, michaelh, ngraham, bruns, 
demsking, cullmann, sars, dhaumann

D17241: WIP: Disable highlighting for lines longer than 1024 characters.

[Kåre Särs]

sars added a comment.


  If we get it working properly we could make the limit much higher. I tried 
with 3 characters and it still worked very good. (except when selecting the 
line)

REPOSITORY
  R39 KTextEditor

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

To: sars, cullmann, vkrause
Cc: brauch, kwrite-devel, kde-frameworks-devel, hase, michaelh, ngraham, bruns, 
demsking, cullmann, sars, dhaumann

D17220: Use red X in disabled and muted status icons consistently

[TrickyRicky]

trickyricky26 updated this revision to Diff 46504.
trickyricky26 added a comment.


  - Apply new off and muted styles; add 22px audio-status icons

REPOSITORY
  R266 Breeze Icons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17220?vs=46419&id=46504

BRANCH
  streamline-muted-off (branched from master)

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

AFFECTED FILES
  icons-dark/status/16/audio-off.svg
  icons-dark/status/16/audio-volume-muted.svg
  icons-dark/status/16/camera-off.svg
  icons-dark/status/16/mic-off.svg
  icons-dark/status/16/microphone-sensitivity-muted.svg
  icons-dark/status/22/audio-off.svg
  icons-dark/status/22/audio-on.svg
  icons-dark/status/22/audio-ready.svg
  icons-dark/status/22/audio-volume-muted.svg
  icons-dark/status/22/camera-off.svg
  icons-dark/status/22/input-touchpad-off.svg
  icons-dark/status/22/mic-off.svg
  icons-dark/status/22/microphone-sensitivity-muted.svg
  icons/status/16/audio-off.svg
  icons/status/16/audio-volume-muted.svg
  icons/status/16/camera-off.svg
  icons/status/16/mic-off.svg
  icons/status/16/microphone-sensitivity-muted.svg
  icons/status/22/audio-off.svg
  icons/status/22/audio-on.svg
  icons/status/22/audio-ready.svg
  icons/status/22/audio-volume-muted.svg
  icons/status/22/camera-off.svg
  icons/status/22/input-touchpad-off.svg
  icons/status/22/mic-off.svg
  icons/status/22/microphone-sensitivity-muted.svg

To: trickyricky26, #vdg, ndavis
Cc: abetts, ndavis, ngraham, kde-frameworks-devel, michaelh, bruns

D17220: Use red X in disabled and muted status icons consistently

[TrickyRicky]

trickyricky26 edited the summary of this revision.
trickyricky26 edited the test plan for this revision.

REPOSITORY
  R266 Breeze Icons

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

To: trickyricky26, #vdg, ndavis
Cc: abetts, ndavis, ngraham, kde-frameworks-devel, michaelh, bruns

D17243: Only cal updateView() in visibleRange() when endPos() is invalid.

[Kåre Särs]

sars added a comment.


  Note: this is especially bad when the "on the fly spellchecking" is enabled 
as visibleRange() is used to "optimize" and only highlight what is currently 
visible. It is called multiple times.

REPOSITORY
  R39 KTextEditor

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

To: sars, cullmann, #kate, dhaumann
Cc: kwrite-devel, kde-frameworks-devel, hase, michaelh, ngraham, bruns, 
demsking, cullmann, sars, dhaumann

D17220: Improve symbolism for off and muted status icon

[TrickyRicky]

trickyricky26 retitled this revision from "Use red X in disabled and muted 
status icons consistently" to "Improve symbolism for off and muted status icon".

REPOSITORY
  R266 Breeze Icons

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

To: trickyricky26, #vdg, ndavis
Cc: abetts, ndavis, ngraham, kde-frameworks-devel, michaelh, bruns

D17220: Improve symbolism for off and muted status icon

[Nathaniel Graham]

ngraham added a comment.


  This stuff looks so good. I'll formally review later. I also think we should 
update the HIG's icons page to officially recommend a red slash for status 
icons' off state and a red circle-with-a-slash-through-it for the muted state. 
All in favor?

REPOSITORY
  R266 Breeze Icons

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

To: trickyricky26, #vdg, ndavis
Cc: abetts, ndavis, ngraham, kde-frameworks-devel, michaelh, bruns

D17245: Add string formatting function to property info

[Alexander Stippich]

astippich created this revision.
astippich added reviewers: broulik, bruns, mgallien.
Herald added projects: Frameworks, Baloo.
Herald added subscribers: Baloo, kde-frameworks-devel.
astippich requested review of this revision.

REVISION SUMMARY
  Adds the ability to format the metadata value 
  for displaying purposes individually for each 
  property. Currently, users of KFileMetaData
  must implement their own custom formatting
  functions.
  All custom formatting functions from
  Baloo-Widgets and Dolphin are copied into
  KFileMetaData. This can be extended later.
  
  This adds a dependency on KCoreAddons to
  KFileMetaData.
  
  FEATURE: 398581

TEST PLAN
  tests pass

REPOSITORY
  R286 KFileMetaData

BRANCH
  display_value

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

AFFECTED FILES
  CMakeLists.txt
  autotests/propertyinfotest.cpp
  autotests/propertyinfotest.h
  src/CMakeLists.txt
  src/formatstrings.cpp
  src/formatstrings.h
  src/propertyinfo.cpp
  src/propertyinfo.h

To: astippich, broulik, bruns, mgallien
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D17245: Add string formatting function to property info

[Alexander Stippich]

astippich updated this revision to Diff 46509.
astippich added a comment.


  - make KCoreAddons a required framework

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17245?vs=46508&id=46509

BRANCH
  display_value

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

AFFECTED FILES
  CMakeLists.txt
  autotests/propertyinfotest.cpp
  autotests/propertyinfotest.h
  src/CMakeLists.txt
  src/formatstrings.cpp
  src/formatstrings.h
  src/propertyinfo.cpp
  src/propertyinfo.h

To: astippich, broulik, bruns, mgallien
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D17245: Add string formatting function to property info

[Nathaniel Graham]

ngraham added inline comments.

INLINE COMMENTS

> formatstrings.cpp:82
> +case 4: string = i18nc("@item:intable image orientation", "Vertically 
> flipped"); break;
> +case 5: string = i18nc("@item:intable image orientation", "Transposed"); 
> break;
> +case 6: string = i18nc("@item:intable image orientation", "90° 
> rotated"); break;

"Transposed" and "Transversed" are very technical terms, and it's hard to 
understand what they mean in this context. I actually have no idea myself! 
Could we consider replacing these terms with plainer language?

REPOSITORY
  R286 KFileMetaData

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

To: astippich, broulik, bruns, mgallien
Cc: ngraham, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, 
spoorun, bruns, abrahams

D16617: fix extraction of GPS altitude for exif data

[Alexander Stippich]

astippich added a comment.


  gentle ping

REPOSITORY
  R286 KFileMetaData

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

To: astippich, #frameworks, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D17220: Improve symbolism for off and muted status icon

[Noah Davis]

ndavis added a comment.


  In D17220#368147 , @trickyricky26 
wrote:
  
  > Should this style then also be applied to other "off" status icons like 
touchpad and camera?
  
  
  I think so. I'm somewhat concerned that on certain icon designs it could make 
them illegible, but I think we find ways to avoid that.
  
  > Also, I've experimented with a circle with one line through it, as that 
represents muted better than an X which looks too much like an error:
  >  F6446578: audio-volume-muted-alt-22.svg.png 

  
  I think this is likely more intuitive than the red dash we currently use.
  
  In D17220#368358 , @ngraham wrote:
  
  > This stuff looks so good. I'll formally review later. I also think we 
should update the HIG's icons page to officially recommend a red slash for 
status icons' off state and a red circle-with-a-slash-through-it for the muted 
state. All in favor?
  
  
  Aye!

REPOSITORY
  R266 Breeze Icons

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

To: trickyricky26, #vdg, ndavis
Cc: abetts, ndavis, ngraham, kde-frameworks-devel, michaelh, bruns

D17220: Improve symbolism for off and muted status icon

[Noah Davis]

ndavis added a comment.


  I'm not sure about this, but I think increasing the size of the red no symbol 
from 6px to 8px on the 22px mute icon might be a good idea. At 6px, it just 
seems to lack visual weight.
  
  6px no symbol
  F6447430: Screenshot_20181129_164625.png 

  
  8px no symbol
  F6447432: Screenshot_20181129_164657.png 


REPOSITORY
  R266 Breeze Icons

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

To: trickyricky26, #vdg, ndavis
Cc: abetts, ndavis, ngraham, kde-frameworks-devel, michaelh, bruns

D17220: Improve symbolism for off and muted status icon

[Noah Davis]

ndavis added a comment.


  Also, the semi-transparent sound waves on the audio icons in the Breeze 
Plasma theme use 25% opacity, not 50% opacity. I think 25% looks better anyway.

REPOSITORY
  R266 Breeze Icons

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

To: trickyricky26, #vdg, ndavis
Cc: abetts, ndavis, ngraham, kde-frameworks-devel, michaelh, bruns

D17220: Improve symbolism for off and muted status icon

[Noah Davis]

ndavis added a comment.


  Also, here's the 16px muted icon with the "No" symbol moved out to the right 
by 1px and then 2px compared to the 16px full volume icon
  
  1px to the right F6447454: Screenshot_20181129_171423.png 

  
  2px to the right F6447452: Screenshot_20181129_171411.png 


REPOSITORY
  R266 Breeze Icons

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

To: trickyricky26, #vdg, ndavis
Cc: abetts, ndavis, ngraham, kde-frameworks-devel, michaelh, bruns

D17200: Avoid writing unchanged data to terms dbs

[Stefan Brüns]

This revision was automatically updated to reflect the committed changes.
Closed by commit R293:aff58c04b5dd: Avoid writing unchanged data to terms dbs 
(authored by bruns).

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17200?vs=46351&id=46515

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

AFFECTED FILES
  src/engine/writetransaction.cpp

To: bruns, #baloo, #frameworks, ngraham, poboiko
Cc: kde-frameworks-devel, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams

D15999: Add icons with added background to system.svgz

[Nathaniel Graham]

ngraham added a reviewer: ndavis.
ngraham added a subscriber: ndavis.
ngraham added a comment.


  Sorry this took a while. I had to stop work on D16031 
 until other changes got accepted and 
merged in, which happened today. Back to these icons...
  
  I hate to keep torturing you here, but even with the latest revision, the 
reboot icon's symbol color is still wrong, and the background circle colors 
seems to be inverted from what they should be:
  
  F6447534: Normal colors.jpeg 
  
  The login screen sets `colorGroup: PlasmaCore.Theme.ComplementaryColorGroup` 
which has the effect of making everything use dark theme colors. If I remove 
that line, here's how it looks:
  
  F6447539: Inverted colors.jpeg 
  
  So now somehow the background circles are changing their colors, but the 
foreground symbols are not!
  
  Do these icons happen to have embedded CSS stylesheets in them that could be 
invoking the automatic color changing  feature and making this happen? Maybe 
@ndavis has an idea?

REPOSITORY
  R242 Plasma Framework (Library)

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

To: pstefan, ngraham, #vdg, ndavis
Cc: ndavis, broulik, filipf, kde-frameworks-devel, michaelh, ngraham, bruns

D15999: Add icons with added background to system.svgz

[Noah Davis]

ndavis added a comment.


  Could these icons be moved into the breeze-icons repo? That would make it 
simpler to edit and review these in the future.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: pstefan, ngraham, #vdg, ndavis
Cc: ndavis, broulik, filipf, kde-frameworks-devel, michaelh, ngraham, bruns

D15999: Add icons with added background to system.svgz

[Nathaniel Graham]

ngraham added a comment.


  The existing icons in this style are already in the Plasma Breeze theme 
though; that was our reasoning for adding them here. In the 
medium-to-long-term, I strongly support deprecating the concept of the Plasma 
Breeze icon theme and centralizing everything in the general Breeze icon theme, 
but I think that's out of scope for this patch.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: pstefan, ngraham, #vdg, ndavis
Cc: ndavis, broulik, filipf, kde-frameworks-devel, michaelh, ngraham, bruns

D17245: Add string formatting function to property info

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> formatstrings.cpp:81
> +case 3: string = i18nc("@item:intable image orientation", "180° 
> rotated"); break;
> +case 4: string = i18nc("@item:intable image orientation", "Vertically 
> flipped"); break;
> +case 5: string = i18nc("@item:intable image orientation", "Transposed"); 
> break;

Thats somewhat ambigous - flipped along the vertical axis, or top/bottom 
flipped?

> ngraham wrote in formatstrings.cpp:82
> "Transposed" and "Transversed" are very technical terms, and it's hard to 
> understand what they mean in this context. I actually have no idea myself! 
> Could we consider replacing these terms with plainer language?

Thats mirroring/flipping along the diagonals. You get the same effect when you 
mirror vertically and rotate +- 90 degrees.

I don't think there is a less technical term.

> formatstrings.cpp:83
> +case 5: string = i18nc("@item:intable image orientation", "Transposed"); 
> break;
> +case 6: string = i18nc("@item:intable image orientation", "90° 
> rotated"); break;
> +case 7: string = i18nc("@item:intable image orientation", 
> "Transversed"); break;

Clockwise or CCW?

REPOSITORY
  R286 KFileMetaData

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

To: astippich, broulik, bruns, mgallien
Cc: ngraham, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, 
spoorun, bruns, abrahams

D15999: Add icons with added background to system.svgz

[Noah Davis]

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


  It appears that the background colors are hardcoded. If they are meant to be 
dark with the Breeze Light Plasma theme and light with Breeze Dark, then they 
should use the `ColorScheme-Text` class and the inner symbols should use the 
`ColorScheme-Background` class.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: pstefan, ngraham, #vdg, ndavis
Cc: ndavis, broulik, filipf, kde-frameworks-devel, michaelh, ngraham, bruns

D15999: Add icons with added background to system.svgz

[Nathaniel Graham]

ngraham added a comment.


  In D15999#368479 , @ndavis wrote:
  
  > It appears that the background colors are hardcoded. If they are meant to 
be dark with the Breeze Light Plasma theme and light with Breeze Dark, then 
they should use the `ColorScheme-Text` class and the inner symbols should use 
the `ColorScheme-Background` class.F6447550: Screenshot_20181129_192418.png 

  
  
  The opposite: the background is supposed to be dark with breeze dark, and 
light with breeze light.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: pstefan, ngraham, #vdg, ndavis
Cc: ndavis, broulik, filipf, kde-frameworks-devel, michaelh, ngraham, bruns

D15999: Add icons with added background to system.svgz

[Filip Fila]

filipf added a comment.


  In D15999#368498 , @ngraham wrote:
  
  > In D15999#368479 , @ndavis wrote:
  >
  > > It appears that the background colors are hardcoded. If they are meant to 
be dark with the Breeze Light Plasma theme and light with Breeze Dark, then 
they should use the `ColorScheme-Text` class and the inner symbols should use 
the `ColorScheme-Background` class.F6447550: Screenshot_20181129_192418.png 

  >
  >
  > The opposite: the background is supposed to be dark with breeze dark, and 
light with breeze light.
  
  
  In general, with dark themes you want white monochrome icons and the circle 
is now the dominant icon element, meaning it should get painted white.
  
  But there is a tangible problem here - imagine what the logout screen would 
look like if the circles were dark:
  
  F6447627: image.png 
  
  They would be weird looking and would be superfluous.
  
  In the case of the SDDM theme I agree it's odd that the circles are colored 
differently than the input box, but the logout screen is more important.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: pstefan, ngraham, #vdg, ndavis
Cc: ndavis, broulik, filipf, kde-frameworks-devel, michaelh, ngraham, bruns

D15999: Add icons with added background to system.svgz

[Nathaniel Graham]

ngraham added a comment.


  In D15999#368499 , @filipf wrote:
  
  > But there is a tangible problem here - imagine what the logout screen when 
using dark themes would look like if the circles were dark[;] They would be 
weird looking and would be superfluous.
  
  
  That's precisely why I wanted a subtle outline around the edge of the 
background circle, which earlier versions had. With that, a dark circle on a 
dark wallpaper didn't become muddy and fade into the wallpaper; it still looked 
crisp and handsome.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: pstefan, ngraham, #vdg, ndavis
Cc: ndavis, broulik, filipf, kde-frameworks-devel, michaelh, ngraham, bruns

D15999: Add icons with added background to system.svgz

[Phil Stefan]

pstefan added a comment.


  In D15999#368499 , @filipf wrote:
  
  > In D15999#368498 , @ngraham 
wrote:
  >
  > > In D15999#368479 , @ndavis 
wrote:
  > >
  > > > It appears that the background colors are hardcoded. If they are meant 
to be dark with the Breeze Light Plasma theme and light with Breeze Dark, then 
they should use the `ColorScheme-Text` class and the inner symbols should use 
the `ColorScheme-Background` class.F6447550: Screenshot_20181129_192418.png 

  > >
  > >
  > > The opposite: the background is supposed to be dark with breeze dark, and 
light with breeze light.
  >
  >
  > In general, with dark themes you want white monochrome icons and the circle 
is now the dominant icon element, meaning it should get painted white.
  >
  > But there is a tangible problem here - imagine what the logout screen when 
using dark themes would look like if the circles were dark:
  >
  > F6447627: image.png 
  >
  > They would be weird looking and would be superfluous.
  >
  > In the case of the SDDM theme I agree it's odd that the circles are colored 
differently than the input box, but the logout screen is more important.
  
  
  The problem here is, that it's not using a dark color-scheme. The login 
screen uses a mix of breeze light/dark items. The
  
  In D15999#368499 , @filipf wrote:
  
  > In D15999#368498 , @ngraham 
wrote:
  >
  > > In D15999#368479 , @ndavis 
wrote:
  > >
  > > > It appears that the background colors are hardcoded. If they are meant 
to be dark with the Breeze Light Plasma theme and light with Breeze Dark, then 
they should use the `ColorScheme-Text` class and the inner symbols should use 
the `ColorScheme-Background` class.F6447550: Screenshot_20181129_192418.png 

  > >
  > >
  > > The opposite: the background is supposed to be dark with breeze dark, and 
light with breeze light.
  >
  >
  > In general, with dark themes you want white monochrome icons and the circle 
is now the dominant icon element, meaning it should get painted white.
  >
  > But there is a tangible problem here - imagine what the logout screen when 
using dark themes would look like if the circles were dark:
  >
  > F6447627: image.png 
  >
  > They would be weird looking and would be superfluous.
  >
  > In the case of the SDDM theme I agree it's odd that the circles are colored 
differently than the input box, but the logout screen is more important.
  
  
  The problem is that it's not a /dark/ theme. Right now we expect the user to 
manually change the theme if they do not like the color of the icons. If we'd 
follow your suggestions the problem would remain, that the white circles would 
look out of place on light backgrounds. We have no mechanism to adjust the 
theme based on the wallpaper's lightness automagically right now.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: pstefan, ngraham, #vdg, ndavis
Cc: ndavis, broulik, filipf, kde-frameworks-devel, michaelh, ngraham, bruns

D17239: ovs-bridge and ovs-interface setting

[Jan Grulich]

jgrulich added inline comments.

INLINE COMMENTS

> ovsbridgesetting.cpp:25
> +#include 
> +
> +NetworkManager::OvsBridgeSettingPrivate::OvsBridgeSettingPrivate()

Add defines for ovs-bridge properties and name in case NM is older than NM 
1.10.0.

> ovsbridgesetting.cpp:27
> +NetworkManager::OvsBridgeSettingPrivate::OvsBridgeSettingPrivate()
> +: name(NM_SETTING_OVS_BRIDGE_SETTING_NAME)
> +, mcastSnoopingEnable(false)

Coding style.

> ovsbridgesetting.cpp:154
> +{
> +dbg.nospace() << "type: " << setting.typeAsString(setting.type()) << 
> '\n';
> +dbg.nospace() << "initialized: " << !setting.isNull() << '\n';

Why this is commented out?

> ovsbridgesetting.h:35
> +/**
> + * Represents generic setting
> + */

Represents ovs-bridge setting

> ovsinterfacesetting.cpp:25
> +#include 
> +
> +NetworkManager::OvsInterfaceSettingPrivate::OvsInterfaceSettingPrivate()

Add define for NM_SETTING_OVS_INTERFACE_SETTING_NAME in case NM is older than 
1.10.0.

> ovsinterfacesetting.cpp:27
> +NetworkManager::OvsInterfaceSettingPrivate::OvsInterfaceSettingPrivate()
> +: name(NM_SETTING_OVS_INTERFACE_SETTING_NAME)
> +{ }

Coding style.

> ovsinterfacesetting.cpp:89
> +{
> +// dbg.nospace() << "type: " << setting.typeAsString(setting.type()) << 
> '\n';
> +dbg.nospace() << "initialized: " << !setting.isNull() << '\n';

Why this is commented out?

> ovsinterfacesetting.h:35
> +/**
> + * Represents generic setting
> + */

Represents ovs-interface setting

REPOSITORY
  R282 NetworkManagerQt

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

To: pranavgade, jgrulich
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D15999: Add icons with added background to system.svgz

[Nathaniel Graham]

ngraham added a comment.


  Urgh, never mind, it was my fault all along. There was some cache somewhere; 
bumping the version number of the plasma theme fixed the issue entirely. I'm 
sorry for wasting your time. :(
  
  I'm happy with this now, though now that it's all working, there are two 
nice-to-have changes that I'd like:
  
  - Restore the subtle outlines around the background circles; those helped 
with contrast against dark backgrounds, and they looked just plain good IMHO
  - Address @ndavis's suggestion and un-hard-code the colors
  
  Thanks so much for your work here, Phil!

REPOSITORY
  R242 Plasma Framework (Library)

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

To: pstefan, ngraham, #vdg, ndavis
Cc: ndavis, broulik, filipf, kde-frameworks-devel, michaelh, ngraham, bruns

D17210: Added proxy and user settings

[Jan Grulich]

jgrulich closed this revision.
jgrulich added a comment.


  Closed by 
https://cgit.kde.org/networkmanager-qt.git/commit/?id=ee74458db6593ed05b89c257de5789d02a0fb64d.

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

To: pranavgade, jgrulich
Cc: cfeck, ngraham, kde-frameworks-devel, michaelh, bruns

KDE CI: Frameworks » plasma-framework » kf5-qt5 SUSEQt5.10 - Build # 273 - Still Unstable!

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks/job/plasma-framework/job/kf5-qt5%20SUSEQt5.10/273/
 Project:
kf5-qt5 SUSEQt5.10
 Date of build:
Fri, 30 Nov 2018 06:58:57 +
 Build duration:
3 min 20 sec and counting
   BUILD ARTIFACTS
  compat_reports/KF5Plasma_compat_report.html
   JUnit Tests
  Name: (root) Failed: 6 test(s), Passed: 9 test(s), Skipped: 0 test(s), Total: 15 test(s)Failed: TestSuite.dialognativetestFailed: TestSuite.plasma-configmodeltestFailed: TestSuite.plasma-fallbackpackagetestFailed: TestSuite.plasma-iconitemtestFailed: TestSuite.plasma-packagestructuretestFailed: TestSuite.plasma-storagetest
   Cobertura Report
  
   Project Coverage Summary
  
   Name
  PackagesFilesClassesLinesConditionalsCobertura Coverage Report33%
(6/18)36%
(45/126)36%
(45/126)27%
(3608/13324)18%
(1817/9848)Coverage Breakdown by Package
Name
   FilesClassesLinesConditionalsautotests86%
(12/14)86%
(12/14)55%
(612/1117)29%
(315/1086)src.declarativeimports.calendar0%
(0/6)0%
(0/6)0%
(0/464)0%
(0/243)src.declarativeimports.core31%
(5/16)31%
(5/16)13%
(299/2248)7%
(96/1456)src.declarativeimports.plasmacomponents0%
(0/6)0%
(0/6)0%
(0/518)0%
(0/207)src.declarativeimports.plasmaextracomponents0%
(0/3)0%
(0/3)0%
(0/42)0%
(0/22)src.declarativeimports.platformcomponents0%
(0/3)0%
(0/3)0%
(0/58)0%
(0/14)src.declarativeimports.platformcomponents.utils0%
(0/2)0%
(0/2)0%
(0/14)0%
(0/2)src.plasma64%
(14/22)64%
(14/22)40%
(1410/3491)28%
(787/2817)src.plasma.packagestructure0%
(0/7)0%
(0/7)0%
(0/134)0%
(0/12)src.plasma.private50%
(9/18)50%
(9/18)43%
(674/1574)29%
(301/1034)src.plasma.scripting0%
(0/3)0%
(0/3)0%
(0/162)0%
(0/128)src.plasmapkg0%
(0/1)0%
(0/1)0%
(0/45)0%
(0/40)src.plasmaquick33%
(4/12)33%
(4/12)29%
(582/2018)18%
(313/1721)src.plasmaquick.private50%
(1/2)50%
(1/2)29%
(31/106)36%
(5/14)src.scriptengines.qml.plasmoid0%
(0/6)0%
(0/6)0%
(0/1178)0%
(0/1028)tests.dpi0%
(0/2)0%
(0/2)0%
(0/21)0%
(0/2)tests.kplugins0%

KDE CI: Frameworks » attica » kf5-qt5 WindowsMSVCQt5.11 - Build # 16 - Unstable!

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks/job/attica/job/kf5-qt5%20WindowsMSVCQt5.11/16/
 Project:
kf5-qt5 WindowsMSVCQt5.11
 Date of build:
Fri, 30 Nov 2018 07:10:46 +
 Build duration:
5 min 15 sec and counting
   JUnit Tests
  Name: (root) Failed: 1 test(s), Passed: 2 test(s), Skipped: 0 test(s), Total: 3 test(s)Failed: TestSuite.providertest

KDE CI: Frameworks » kcoreaddons » kf5-qt5 SUSEQt5.9 - Build # 74 - Still Unstable!

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks/job/kcoreaddons/job/kf5-qt5%20SUSEQt5.9/74/
 Project:
kf5-qt5 SUSEQt5.9
 Date of build:
Fri, 30 Nov 2018 07:17:57 +
 Build duration:
4 min 22 sec and counting
   BUILD ARTIFACTS
  compat_reports/KF5CoreAddons_compat_report.html
   JUnit Tests
  Name: (root) Failed: 1 test(s), Passed: 23 test(s), Skipped: 0 test(s), Total: 24 test(s)Failed: TestSuite.kdirwatch_qfswatch_unittest
   Cobertura Report
  
   Project Coverage Summary
  
   Name
  PackagesFilesClassesLinesConditionalsCobertura Coverage Report91%
(10/11)85%
(70/82)85%
(70/82)75%
(6347/8463)43%
(10115/23354)Coverage Breakdown by Package
Name
   FilesClassesLinesConditionalsautotests93%
(26/28)93%
(26/28)97%
(2697/2772)49%
(5766/11827)src.desktoptojson100%
(2/2)100%
(2/2)77%
(86/111)38%
(121/321)src.lib67%
(2/3)67%
(2/3)60%
(353/591)26%
(243/938)src.lib.caching100%
(2/2)100%
(2/2)45%
(354/784)18%
(184/1044)src.lib.io82%
(9/11)82%
(9/11)68%
(854/1265)37%
(1043/2803)src.lib.jobs71%
(5/7)71%
(5/7)57%
(160/281)38%
(52/138)src.lib.plugin100%
(7/7)100%
(7/7)85%
(660/776)42%
(972/2301)src.lib.randomness100%
(2/2)100%
(2/2)70%
(67/96)58%
(44/76)src.lib.text63%
(5/8)63%
(5/8)48%
(377/787)45%
(863/1927)src.lib.util100%
(10/10)100%
(10/10)81%
(739/915)52%
(827/1581)tests0%
(0/2)0%
(0/2)0%
(0/85)0%
(0/398)

KDE CI: Frameworks » kcoreaddons » kf5-qt5 SUSEQt5.10 - Build # 130 - Still Unstable!

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks/job/kcoreaddons/job/kf5-qt5%20SUSEQt5.10/130/
 Project:
kf5-qt5 SUSEQt5.10
 Date of build:
Fri, 30 Nov 2018 07:17:56 +
 Build duration:
10 min and counting
   BUILD ARTIFACTS
  compat_reports/KF5CoreAddons_compat_report.html
   JUnit Tests
  Name: (root) Failed: 1 test(s), Passed: 23 test(s), Skipped: 0 test(s), Total: 24 test(s)Failed: TestSuite.kdirwatch_qfswatch_unittest
   Cobertura Report
  
   Project Coverage Summary
  
   Name
  PackagesFilesClassesLinesConditionalsCobertura Coverage Report91%
(10/11)85%
(70/82)85%
(70/82)75%
(6345/8463)43%
(10112/23358)Coverage Breakdown by Package
Name
   FilesClassesLinesConditionalsautotests93%
(26/28)93%
(26/28)97%
(2696/2772)49%
(5762/11825)src.desktoptojson100%
(2/2)100%
(2/2)77%
(86/111)38%
(122/325)src.lib67%
(2/3)67%
(2/3)60%
(352/591)26%
(243/942)src.lib.caching100%
(2/2)100%
(2/2)45%
(354/784)18%
(184/1044)src.lib.io82%
(9/11)82%
(9/11)68%
(854/1265)37%
(1041/2797)src.lib.jobs71%
(5/7)71%
(5/7)57%
(160/281)38%
(52/138)src.lib.plugin100%
(7/7)100%
(7/7)85%
(660/776)42%
(972/2303)src.lib.randomness100%
(2/2)100%
(2/2)70%
(67/96)58%
(44/76)src.lib.text63%
(5/8)63%
(5/8)48%
(377/787)45%
(864/1927)src.lib.util100%
(10/10)100%
(10/10)81%
(739/915)52%
(828/1583)tests0%
(0/2)0%
(0/2)0%
(0/85)0%
(0/398)

KDE CI: Frameworks » kpackage » kf5-qt5 SUSEQt5.10 - Build # 90 - Still Unstable!

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks/job/kpackage/job/kf5-qt5%20SUSEQt5.10/90/
 Project:
kf5-qt5 SUSEQt5.10
 Date of build:
Fri, 30 Nov 2018 07:51:14 +
 Build duration:
1 min 22 sec and counting
   JUnit Tests
  Name: (root) Failed: 6 test(s), Passed: 6 test(s), Skipped: 0 test(s), Total: 12 test(s)Failed: TestSuite.testfallbackpackage-appstreamFailed: TestSuite.testjsonmetadatapackage-appstreamFailed: TestSuite.testpackage-appstreamFailed: TestSuite.testpackage-nodisplay-appstreamFailed: TestSuite.testpackagesdep-appstreamFailed: TestSuite.testpackagesdepinvalid-appstream
   Cobertura Report
  
   Project Coverage Summary
  
   Name
  PackagesFilesClassesLinesConditionalsCobertura Coverage Report100%
(5/5)95%
(20/21)95%
(20/21)72%
(1568/2166)51%
(1079/2124)Coverage Breakdown by Package
Name
   FilesClassesLinesConditionalsautotests100%
(6/6)100%
(6/6)99%
(531/533)51%
(242/470)autotests.mockdepresolver100%
(1/1)100%
(1/1)78%
(14/18)58%
(7/12)src.kpackage75%
(3/4)75%
(3/4)74%
(557/753)64%
(598/935)src.kpackage.private100%
(7/7)100%
(7/7)79%
(302/380)49%
(104/211)src.kpackagetool100%
(3/3)100%
(3/3)34%
(164/482)26%
(128/496)

KDE CI: Frameworks » kpackage » kf5-qt5 SUSEQt5.9 - Build # 68 - Still Unstable!

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks/job/kpackage/job/kf5-qt5%20SUSEQt5.9/68/
 Project:
kf5-qt5 SUSEQt5.9
 Date of build:
Fri, 30 Nov 2018 07:51:14 +
 Build duration:
5 min 6 sec and counting
   JUnit Tests
  Name: (root) Failed: 6 test(s), Passed: 6 test(s), Skipped: 0 test(s), Total: 12 test(s)Failed: TestSuite.testfallbackpackage-appstreamFailed: TestSuite.testjsonmetadatapackage-appstreamFailed: TestSuite.testpackage-appstreamFailed: TestSuite.testpackage-nodisplay-appstreamFailed: TestSuite.testpackagesdep-appstreamFailed: TestSuite.testpackagesdepinvalid-appstream
   Cobertura Report
  
   Project Coverage Summary
  
   Name
  PackagesFilesClassesLinesConditionalsCobertura Coverage Report100%
(5/5)95%
(20/21)95%
(20/21)72%
(1568/2166)51%
(1079/2124)Coverage Breakdown by Package
Name
   FilesClassesLinesConditionalsautotests100%
(6/6)100%
(6/6)99%
(531/533)51%
(242/470)autotests.mockdepresolver100%
(1/1)100%
(1/1)78%
(14/18)58%
(7/12)src.kpackage75%
(3/4)75%
(3/4)74%
(557/753)64%
(598/935)src.kpackage.private100%
(7/7)100%
(7/7)79%
(302/380)49%
(104/211)src.kpackagetool100%
(3/3)100%
(3/3)34%
(164/482)26%
(128/496)