D3987: Use nullptr in all Frameworks (just diff in KIO shown here)

[Kevin Funk]

kfunk added a comment.


  In https://phabricator.kde.org/D3987#96418, @kossebau wrote:
  
  > In https://phabricator.kde.org/D3987#95858, @kfunk wrote:
  >
  > > Closing. This Diff refactored the code technically correct.
  >
  >
  > Technically correct, as in: it builds.
  >  But semantically it is incorrect and a regression when it comes to flags, 
especially as high level languages are made for humans in the first line, not 
compilers ;)
  
  
  Not true, the patch does /not/ change behavior. See explanation in 
https://phabricator.kde.org/D3987#78426.
  
  >> If you want to replace `MyFlags flags = nullptr` by `... = {}` please do 
so in a follow-up patch/review.
  > 
  > Seems you do not see yourself in the duty to do the fix-up. so I have to 
scratch this itch myself, tss :)
  
  Yes, honestly I think either way is fine (`nullptr` or `{}`). Get... used to 
the new look I'd say ;)
  
  > So please tell what tools you used to create this diff, so I could have a 
look how I can reuse them for fixing all the `MyFlags flags = nullptr` by `... 
= {}`? Any suggestions how that could be done best?
  
  clang-tidy was being used. You need to special case its 
'modernize-use-nullptr' check for `QFlags`, fwiw...

REPOSITORY
  R280 Prison

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

To: kfunk, #frameworks, dfaure, kossebau
Cc: skelly, kossebau, dfaure, graesslin

D3987: Use nullptr in all Frameworks (just diff in KIO shown here)

[Friedrich W. H. Kossebau]

kossebau added a comment.


  In https://phabricator.kde.org/D3987#95858, @kfunk wrote:
  
  > Closing. This Diff refactored the code technically correct.
  
  
  Technically correct, as in: it builds.
  But semantically it is incorrect and a regression when it comes to flags, 
especially as high level languages are made for humans in the first line, not 
compilers ;)
  
  > If you want to replace `MyFlags flags = nullptr` by `... = {}` please do so 
in a follow-up patch/review.
  
  Seems you do not see yourself in the duty to do the fix-up. so I have to 
scratch this itch myself, tss :)
  So please tell what tools you used to create this diff, so I could have a 
look how I can reuse them for fixing all the `MyFlags flags = nullptr` by `... 
= {}`? Any suggestions how that could be done best?

REPOSITORY
  R280 Prison

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

To: kfunk, #frameworks, dfaure, kossebau
Cc: skelly, kossebau, dfaure, graesslin

D5111: Provide demo/preview for checkable menu items and colour scheme comparison

[René J . V . Bertin]

rjvbb updated this revision to Diff 12647.
rjvbb added a comment.


  stripped diff.

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D5111?vs=12644&id=12647

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

AFFECTED FILES
  kstyle/demo/oxygendemodialog.cpp
  kstyle/demo/oxygendemodialog.h
  kstyle/demo/oxygenmdidemowidget.cpp

To: rjvbb, jriddell, anthonyfieroni, zhigalin, hpereiradacosta
Cc: kde-mac, #frameworks

D5111: Provide demo/preview for checkable menu items and colour scheme comparison

[René J . V . Bertin]

rjvbb added a comment.


  Colour scheme chooser proposition split off: https://phabricator.kde.org/D5113

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

To: rjvbb, jriddell, anthonyfieroni, zhigalin, hpereiradacosta
Cc: kde-mac, #frameworks

D5111: Provide demo/preview for checkable menu items and colour scheme comparison

[René J . V . Bertin]

rjvbb updated this revision to Diff 12644.
rjvbb edited the summary of this revision.
rjvbb added a comment.


  Updated, not yet split.
  
  I've kept the former left-to-right menu action as a "check here" noop action 
and made the distinction a bit more explicit by adding a menu section (which 
also didn't didn't benefit from a preview yet).
  
  FWIW: notice how Qt flips "<- Check here" to "Check here ->" automatically 
when flipping the layout! Should a comment about this feature be added so that 
translators will preserve it?

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D5111?vs=12634&id=12644

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

AFFECTED FILES
  kstyle/demo/CMakeLists.txt
  kstyle/demo/oxygendemodialog.cpp
  kstyle/demo/oxygendemodialog.h
  kstyle/demo/oxygenmdidemowidget.cpp
  kstyle/demo/oxygenschemechooser.cpp
  kstyle/demo/oxygenschemechooser.h

To: rjvbb, jriddell, anthonyfieroni, zhigalin, hpereiradacosta
Cc: kde-mac, #frameworks

D5111: Provide demo/preview for checkable menu items and colour scheme comparison

[René J . V . Bertin]

rjvbb added a comment.


  Once more. Note how Oxygen renders an icon on the menu button despite me 
having disabled the icons-in-buttons feature in the settings. Only QtCurve 
seems to respect this setting for regular buttons (in dialog button boxes) 
nowadays.
  
  F2988220: oxydemo-oxygen-menu.png 

REPOSITORY
  R113 Oxygen Theme

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

To: rjvbb, jriddell, anthonyfieroni, zhigalin, hpereiradacosta
Cc: kde-mac, #frameworks

D5111: Provide demo/preview for checkable menu items and colour scheme comparison

[René J . V . Bertin]

rjvbb added a comment.


  Screenshots:
  
  F2988075: oxydemo-breeze.png 
  
  F2988076: oxydemo-qtcurve.png 
  
  F2988077: oxydemo-oxygen.png 
  
  F2988078: oxydemo-qtcurve-menu.png 

REPOSITORY
  R113 Oxygen Theme

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

To: rjvbb, jriddell, anthonyfieroni, zhigalin, hpereiradacosta
Cc: kde-mac, #frameworks

D5111: Provide demo/preview for checkable menu items and colour scheme comparison

[René J . V . Bertin]

rjvbb added a comment.


  > - please re-add the screenshot from Review Board. (sorry I was not aware of 
this review request cause I was not in the list of reviewers, even though 
official maintainer of oxygen ...)
  
  Sorry about that, I thought you'd be a member of the Plasma group. But I had 
a hunch so added a few reviewers manually this time.
  
  > - right to left layout action <- no. There is already one at the bottom of 
the window.
  
  The goal was not to duplicate functionality, but to add an additional item to 
the Layout menu that was not mutually exclusive with the others, as different 
styles can render such items differently. It also gives the possibility to add 
a separator.
  IIRC the patch *would* become somewhat simpler if I just leave the menu item 
but make it a noop. Would you be OK with that?
  
  > - this review should really be several, one per feature: one for the 
checkboxes/radiobuttons in the mdi window, one for the colorschemechooser. Can 
you split ?
  
  Yay, extra work... Fortunately not too much, will do later today.
  
  > - finally, there is need for more detail review (once above is done). For 
instance, in ColorSchemeChooser you use SUPPORT_THEME_SAVING, but this one is 
set/defined nowhere.
  
  TBH, I was more or less hoping for feedback like this. I also don't see the 
need to save this setting but didn't want to strip it out immediately, also out 
of some form of respect for Alexander's original work.

REPOSITORY
  R113 Oxygen Theme

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

To: rjvbb, jriddell, anthonyfieroni, zhigalin, hpereiradacosta
Cc: kde-mac, #frameworks

D5111: Provide demo/preview for checkable menu items and colour scheme comparison

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  Hi,
  Thanks for the set of patches.
  
  in general, i am ok with the change but:
  
  - please re-add the screenshot from Review Board. (sorry I was not aware of 
this review request cause I was not in the list of reviewers, even though 
official maintainer of oxygen ...)
  - right to left layout action <- no. There is already one at the bottom of 
the window.
  - this review should really be several, one per feature: one for the 
checkboxes/radiobuttons in the mdi window, one for the colorschemechooser. Can 
you split ?
  - finally, there is need for more detail review (once above is done). For 
instance, in ColorSchemeChooser you use SUPPORT_THEME_SAVING, but this one is 
set/defined nowhere. So the whole corresponding code should go, right ? Or is 
it work in progress ? Personally I would disagree with having oxygen-demo being 
anything other than a demo, and for instance altering configuration. This is 
not the right place. The right place is the relevant KCM dialog.
  
  Best,
  
  Hugo

REPOSITORY
  R113 Oxygen Theme

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

To: rjvbb, jriddell, anthonyfieroni, zhigalin, hpereiradacosta
Cc: kde-mac, #frameworks

D5088: Generate plugins.qmltypes files for the plugins we install

[Aleix Pol Gonzalez]

apol added a comment.


  In https://phabricator.kde.org/D5088#96296, @davidedmundson wrote:
  
  > I'm confused.
  >
  > If you're generating the qmltypes with the ecm command, why are they in the 
diff?
  
  
  Good point.
  We can't generate them at runtime. Here's how they put it in Qt:
  
# plugins.qmltypes is used by Qt Creator for syntax highlighting and the 
QML code model.  It needs
# to be regenerated whenever the QML elements exported by the plugin 
change.  This cannot be done
# automatically at compile time because qmlplugindump does not support some 
QML features and it may
# not be possible when cross-compiling.
  
  Biggest issue is that you need a set up install prefix to generate the 
plugins as it expects the prefix structure. Also it needs to load the plugin so 
it will certainly be a problem on certain setups.
  
  We could find a workaround where it gets to be generated at build-time, but 
it will feel off. Here at least we're doing the same Qt does with their 
`plugins.qmltypes` files.

REPOSITORY
  R242 Plasma Framework (Library)

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

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

D5111: Provide demo/preview for checkable menu items and colour scheme comparison

[René J . V . Bertin]

rjvbb removed a project: Plasma.
rjvbb removed a subscriber: plasma-devel.

REPOSITORY
  R113 Oxygen Theme

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

To: rjvbb, hpereiradacosta, jriddell, anthonyfieroni, zhigalin
Cc: kde-mac, #frameworks

D5111: Provide demo/preview for checkable menu items and colour scheme comparison

[René J . V . Bertin]

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

REVISION SUMMARY
  This is a continuation/transfer of https://git.reviewboard.kde.org/r/128109/ 
(which still has a few screenshots)
  
  There currently is no "official" cheap way to preview how checked menu items 
are rendered as a function of installed widget style, nor an easy/cheap way to 
compare the look of a given widget style under different colour schemes. I have 
been annoyed by the fact one has to (re)launch often heavy applications to 
assess the impact of changes to these look-and-feel aspects.
  
  This patch explores a straightforward way of adding that missing 
functionality via the most exhaustive look-and-feel preview utility I am aware 
of that is cheap in resource requirements: Oxygen's demo app.
  
  This demo already has menus in the MDI preview. The patch makes the layout 
items checkable and puts them in a QActionGroup so their mutually exclusive 
nature is taken into account. It also adds a "right to left" non-exclusive 
checkable item which is linked to the "Left to Right" checkbox in the demo 
frame.
  
  For colour scheme exploration I adapted a thin wrapper around 
KColorSchemeManager written originally by Alexander Zhigalin for KDevelop; it 
is used to add a button with drop-down menu to the page widget's button box 
which makes it available in all views.

TEST PLAN
  The menu additions have been checked on Linux and Mac OS X with Qt 5.6.0 - 
5.8.0 and frameworks 5.22.0 - 5.32.0; the colour scheme addition with Qt 5.8.0 
and frameworks 5.32.0 .
  
  I looked for an appropriate way to uncheck the Tile/Cascade/Tabbed menu items 
(= when the user moves or resizes one of the MDI windows) but it seems those 
events are not available through signals and would thus require subclassing 
MdiSubWindow .
  
  It would make sense IMHO to separate this demo from Oxygen. It has nothing 
Oxygen-specific and could be provided as a standalone theme explorer or bundled 
with another package that doesn't have too strong connotations of being 
"Plasma-exclusive" (Qt has widget style and palette support on most if not all 
platforms it runs on).

REPOSITORY
  R113 Oxygen Theme

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

AFFECTED FILES
  kstyle/demo/CMakeLists.txt
  kstyle/demo/oxygendemodialog.cpp
  kstyle/demo/oxygendemodialog.h
  kstyle/demo/oxygenmdidemowidget.cpp
  kstyle/demo/oxygenmdidemowidget.h
  kstyle/demo/oxygenschemechooser.cpp
  kstyle/demo/oxygenschemechooser.h

To: rjvbb, hpereiradacosta, jriddell, anthonyfieroni, zhigalin
Cc: kde-mac, plasma-devel, #frameworks, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol

D5088: Generate plugins.qmltypes files for the plugins we install

[David Edmundson]

davidedmundson added a comment.


  I'm confused.
  
  If you're generating the qmltypes with the ecm command, why are they in the 
diff?

REPOSITORY
  R242 Plasma Framework (Library)

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

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

D5088: Generate plugins.qmltypes files for the plugins we install

[Eike Hein]

hein added a comment.


  Might be nice to do for p-w as well (libtaskmanager QML plugin).

REPOSITORY
  R242 Plasma Framework (Library)

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

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

D5079: debug when disabling kcrash due to env vars

[Harald Sitter]

This revision was automatically updated to reflect the committed changes.
Closed by commit R285:c916d7b8f3a2: debug when disabling kcrash due to env vars 
(authored by sitter).

REPOSITORY
  R285 KCrash

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D5079?vs=12553&id=12631

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

AFFECTED FILES
  src/kcrash.cpp

To: sitter, dfaure, mpyne
Cc: mpyne, #frameworks

Jenkins-kde-ci: kio master kf5-qt5 » Linux,gcc - Build # 474 - Failure!

[no-reply]

GENERAL INFO

BUILD FAILURE
Build URL: 
https://build.kde.org/job/kio%20master%20kf5-qt5/PLATFORM=Linux,compiler=gcc/474/
Project: PLATFORM=Linux,compiler=gcc
Date of build: Mon, 20 Mar 2017 06:58:58 +
Build duration: 3 min 2 sec

CHANGE SET
Revision b6b1c0a6aac2c176491ff125e0062fb557476590 by yurchor: (Fix minor typos)
  change: edit src/widgets/accessmanager.h
  change: edit src/kcms/kio/useragentdlg.cpp
  change: edit src/ioslaves/http/shoutcast-icecast.txt
  change: edit src/core/tcpslavebase.cpp
  change: edit src/kcms/kio/smbrodlg.cpp
  change: edit src/filewidgets/kurlnavigatorbuttonbase_p.h
  change: edit src/core/kfileitem.h
  change: edit src/core/klocalsocket.cpp
  change: edit src/core/transferjob.h

Jenkins-kde-ci: kio master stable-kf5-qt5 » Linux,gcc - Build # 478 - Failure!

[no-reply]

GENERAL INFO

BUILD FAILURE
Build URL: 
https://build.kde.org/job/kio%20master%20stable-kf5-qt5/PLATFORM=Linux,compiler=gcc/478/
Project: PLATFORM=Linux,compiler=gcc
Date of build: Mon, 20 Mar 2017 06:58:58 +
Build duration: 3 min 6 sec

CHANGE SET
Revision b6b1c0a6aac2c176491ff125e0062fb557476590 by yurchor: (Fix minor typos)
  change: edit src/ioslaves/http/shoutcast-icecast.txt
  change: edit src/kcms/kio/useragentdlg.cpp
  change: edit src/core/transferjob.h
  change: edit src/widgets/accessmanager.h
  change: edit src/core/tcpslavebase.cpp
  change: edit src/kcms/kio/smbrodlg.cpp
  change: edit src/core/kfileitem.h
  change: edit src/core/klocalsocket.cpp
  change: edit src/filewidgets/kurlnavigatorbuttonbase_p.h