D15645: Add scheme selection menu with a "System" entry.

[Amish Naidu]

amhndu marked an inline comment as done.

REPOSITORY
  R265 KConfigWidgets

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

To: amhndu, #frameworks, broulik, cfeck, elvisangelaccio, ngraham
Cc: ngraham, broulik, kde-frameworks-devel, michaelh, bruns

D15645: Add scheme selection menu with a "System" entry.

[Amish Naidu]

amhndu updated this revision to Diff 44279.
amhndu edited the summary of this revision.
amhndu added a comment.


  Fixed error in colorschemedemo test.
  Sorry about that, I must've forgotton to commit it.

REPOSITORY
  R265 KConfigWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15645?vs=42166=44279

BRANCH
  system-default

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

AFFECTED FILES
  src/kcolorschememanager.cpp
  src/kcolorschememanager.h
  src/kcolorschememanager_p.h
  tests/kcolorschemedemo.cpp

To: amhndu, #frameworks, broulik, cfeck, elvisangelaccio, ngraham
Cc: ngraham, broulik, kde-frameworks-devel, michaelh, bruns

D16416: z/OS CLIST file syntax highlighting

[Phil Young]

phily added a comment.


  Example CLIST can be viewed here: 
https://gist.githubusercontent.com/mainframed/9e9c85b7a13186e7ca51b5b47d90b575/raw/1e19fede7d203585b213d2f20700ed08baac9e07/protall.clist
  
  Looking at the code repo I wasn't sure exactly where to include the code 
examples.

REPOSITORY
  R216 Syntax Highlighting

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

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

D16416: z/OS CLIST file syntax highlighting

[Phil Young]

phily updated this revision to Diff 44278.
phily marked 5 inline comments as done.
phily added a comment.


  Fixing issues noted by reviewer

REPOSITORY
  R216 Syntax Highlighting

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16416?vs=44182=44278

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

AFFECTED FILES
  data/syntax/clist.xml

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

D16415: Creating new syntax highlighting file for Job Control Language (JCL)

[Phil Young]

phily marked an inline comment as done.
phily added a comment.


  Thank you for the feedback. I've gone and updated the syntax file to explain 
what JCL is and remove the `^` from the regex. Thank you for the suggestions.

REPOSITORY
  R216 Syntax Highlighting

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

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

D16415: Creating new syntax highlighting file for Job Control Language (JCL)

[Phil Young]

phily updated this revision to Diff 44277.
phily marked an inline comment as done.
phily added a comment.


  Making changes based on reviewer comments.

REPOSITORY
  R216 Syntax Highlighting

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16415?vs=44181=44277

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

AFFECTED FILES
  data/syntax/jcl.xml

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

D16421: Improve emblem contrast, legibility and consistency

[Noah Davis]

ndavis added a comment.


  In D16421#349083 , @ngraham wrote:
  
  > In D16421#349082 , @ndavis wrote:
  >
  > > In D16421#349081 , @ngraham 
wrote:
  > >
  > > > This would all require some additional changes in Dolphin and Folder 
view, of course.
  > >
  > >
  > > So should I change `emblem-added` and `emblem-remove` back to their 
original colors? It wouldn't be any worse than it currently is for Dolphin 
before any changes are applied to it.
  >
  >
  > Well, we should discuss that first. :)
  >
  > An LXR search 
 reveals that all 
users of these emblem use them in the same way. We have two options for how we 
want to change things:
  >
  > - Use new iconography (such as my proposed checkmark) for `emblem-added` 
and `emblem-remove`
  > - Create new emblems for this and then over time change our apps to use 
them. This seems less desirable since then `emblem-added` and `emblem-remove` 
will then just be unused, and also it requires code changes to those apps.
  >
  >   So if we want to further improve these emblems, my vote is for changing 
the existing icons rather than making new ones.
  
  
  So keep the blue color for now?
  
  Something that I plan to do in the future is re-do the package management 
icons and I was hoping to reuse emblem-added and emblem-removed for that. 
Currently, the package management icons are action icons, but they are hard to 
read and they blur the line between action and state the way they are used, 
which is why I'm planning to make them emblems instead.

REPOSITORY
  R266 Breeze Icons

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

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

D16421: Improve emblem contrast, legibility and consistency

[Nathaniel Graham]

ngraham added a comment.


  In D16421#349082 , @ndavis wrote:
  
  > In D16421#349081 , @ngraham 
wrote:
  >
  > > This would all require some additional changes in Dolphin and Folder 
view, of course.
  >
  >
  > So should I change `emblem-added` and `emblem-remove` back to their 
original colors? It wouldn't be any worse than it currently is for Dolphin 
before any changes are applied to it.
  
  
  Well, we should discuss that first. :)
  
  An LXR search  
reveals that all users of these emblem use them in the same way. We have two 
options for how we want to change things:
  
  - Use new iconography (such as my proposed checkmark) for `emblem-added` and 
`emblem-remove`
  - Create new emblems for this and then over time change our apps to use them. 
This seems less desirable since then `emblem-added` and `emblem-remove` will 
then just be unused, and also it requires code changes to those apps.
  
  So if we want to further improve these emblems, my vote is for changing the 
existing icons rather than making new ones.

REPOSITORY
  R266 Breeze Icons

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

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

D16421: Improve emblem contrast, legibility and consistency

[Noah Davis]

ndavis added a comment.


  In D16421#349081 , @ngraham wrote:
  
  > This would all require some additional changes in Dolphin and Folder view, 
of course.
  
  
  So should I change `emblem-added` and `emblem-remove` back to their original 
colors? It wouldn't be any worse than it currently is for Dolphin before any 
changes are applied to it.

REPOSITORY
  R266 Breeze Icons

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

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

D16421: Improve emblem contrast, legibility and consistency

[Nathaniel Graham]

ngraham added a comment.


  In D16421#349080 , @ndavis wrote:
  
  > Something just occurred to me: Why do we use emblems as if they were action 
buttons in Dolphin? Would it be so bad if Dolphin used `list-add` and 
`list-remove` instead of `emblem-added` and `emblem-remove`?
  
  
  I had the same thought today. Perhaps we should have them look like 
checkboxes: when you hover over the icon, you see an empty checkbox, and when 
you click on it, it becomes a checked checkbox. This would be more semantically 
correct and also re-use people's existing familiarity with the concept of 
checkboxes. If we do this, I'd also like to see the checked checkbox remain 
visible when the icon is selected. This is how item selection is typically 
handled on mobile operating systems as well, and it's nice and clear.
  
  This would all require some additional changes in Dolphin and Folder view, of 
course.

REPOSITORY
  R266 Breeze Icons

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

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

D16421: Improve emblem contrast, legibility and consistency

[Noah Davis]

ndavis added a comment.


  Something just occurred to me: Why do we use emblems as if they were action 
buttons in Dolphin? Would it be so bad if Dolphin used `list-add` and 
`list-remove` instead of `emblem-added` and `emblem-remove`?

REPOSITORY
  R266 Breeze Icons

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

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

D16421: Improve emblem contrast, legibility and consistency

[Nathaniel Graham]

ngraham added a comment.


  In D16421#349075 , @bruns wrote:
  
  > There may also be a conceptual issue here - previously, the `emblem-locked` 
which is used for unaccessible (e.g. root owned) files was a lock with orange 
background, now it is green. For locked encrypted files green may be suitable, 
for unacessible files not so much.
  
  
  That's true. Probably we need a new emblem `emblem-readonly` that can be used 
for read-only files and directories. It's always been kind of an abuse of the 
iconography to re-use the lock for this. Only now as we're better defining the 
visuals of the lock emblem does it become really obvious.

REPOSITORY
  R266 Breeze Icons

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

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

D16421: Improve emblem contrast, legibility and consistency

[Noah Davis]

ndavis added a comment.


  In D16421#349075 , @bruns wrote:
  
  > @ndavis - can you upload the `git format-patch -1` output somewhere 
(temporary)?
  
  
  https://hastebin.com/egepiwurab.diff
  
  > There may also be a conceptual issue here - previously, the `emblem-locked` 
which is used for unaccessible (e.g. root owned) files was a lock with orange 
background, now it is green. For locked encrypted files green may be suitable, 
for unacessible files not so much.
  
  You're right. Perhaps we should introduce some new emblems?
  
  Some ideas:
  
  `emblem-select` (`emblem-added` sounds like it should be used for things that 
were successfully added)
  `emblem-deselect` (in some contexts, `emblem-remove` would be destructive)
  `emblem-encrypted-locked` (to match the existing `emblem-encrypted-unlocked`, 
Papirus has this)
  `emblem-readonly` (Papirus has this)
  
  Here's a list of emblems from the Papirus theme: 
https://hastebin.com/asuquwoyap.css
  I'm not saying we should use all of these, but we can use that list to pick 
compatible names and having icons that match the context is a good thing.

REPOSITORY
  R266 Breeze Icons

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

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

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

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks/job/kiconthemes/job/kf5-qt5%20WindowsMSVCQt5.11/16/
 Project:
kf5-qt5 WindowsMSVCQt5.11
 Date of build:
Fri, 26 Oct 2018 23:42:12 +
 Build duration:
13 min and counting
   JUnit Tests
  Name: (root) Failed: 1 test(s), Passed: 6 test(s), Skipped: 0 test(s), Total: 7 test(s)Failed: TestSuite.kiconloader_unittest

D16446: [KIconLoader] Replace heap-allocated QImageReader with stack-allocated one

[Stefan Brüns]

This revision was automatically updated to reflect the committed changes.
Closed by commit R302:34994ba9890d: [KIconLoader] Replace heap-allocated 
QImageReader with stack-allocated one (authored by bruns).

REPOSITORY
  R302 KIconThemes

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16446?vs=44275=44276

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

AFFECTED FILES
  src/kiconloader.cpp

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

D16421: Improve emblem contrast, legibility and consistency

[Stefan Brüns]

bruns added a comment.


  In D16421#349072 , @ngraham wrote:
  
  > After manual application, the alignment is perfect for me too. So that's 
not a blocker; must be some local issye on your end, @ndavis. I think we're 
really close (or ready?) to being able to land this! @bruns, are your comments 
about the question mark a blocker, or are you satisfied?
  
  
  
  
  > I've pinged #syadmin about being unable to apply the patch; hopefully they 
can help us out!
  
  @ndavis - can you upload the `git format-patch -1` output somewhere 
(temporary)?
  
  In D16421#348960 , @ngraham wrote:
  
  > In D16421#348959 , @bruns wrote:
  >
  > > Tried to reproduce it here, but for me the bottom-right emblem just 
renders fine here:
  > >
  > > F6363939: Unpatched_locked_link_48.png 

  >
  
  
  There may also be a conceptual issue here - previously, the `emblem-locked` 
which is used for unaccessible (e.g. root owned) files was a lock with orange 
background, now it is green. For locked encrypted files green may be suitable, 
for unacessible files not so much.

REPOSITORY
  R266 Breeze Icons

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

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

D16446: [KIconLoader] Replace heap-allocated QImageReader with stack-allocated one

[Aleix Pol Gonzalez]

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

REPOSITORY
  R302 KIconThemes

BRANCH
  master

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

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

D16421: Improve emblem contrast, legibility and consistency

[Nathaniel Graham]

ngraham added a comment.


  After manual application, the alignment is perfect for me too. So that's not 
a blocker; must be some local issye on your end, @ndavis. I think we're really 
close (or ready?) to being able to land this! @bruns, are your comments about 
the question mark a blocker, or are you satisfied?
  
  I've pinged #syadmin about being unable to apply the patch; hopefully they 
can help us out!

REPOSITORY
  R266 Breeze Icons

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

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

D16344: Do not try to fallback to "less secure" protocols

[Albert Astals Cid]

aacid added a comment.


  In D16344#348985 , @stikonas wrote:
  
  > Can you confirm that it works with TLSv1.2 only sites? (e.g. 
https://stikonas.eu:5281/admin/). Ideally we should  test with TLSv1.3 too.
  
  
  Yes, it works. And https://tls13.crypto.mozilla.org/ works too (on Konqueror 
using KHTML, sadly not on falkon because i guess it doesn't use the kioslave 
infrastructure, but that's a different story i guess)

REPOSITORY
  R241 KIO

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

To: aacid
Cc: jtamate, carewolf, dfaure, stikonas, kde-frameworks-devel, michaelh, 
ngraham, bruns

D16344: Do not try to fallback to "less secure" protocols

[Albert Astals Cid]

aacid added a comment.


  In D16344#348886 , @jtamate wrote:
  
  > What protocol does KTcpSocket::SecureProtocols implement (I can't guess 
it)? If it is the same as QSsl:SecureProtocols 
  
  
  Yes, see ./src/core/ktcpsocket.cpp:89:
  
  > it does:
  >  On the client side, this will send a TLS 1.0 Client Hello, enabling 
TLSv1_0 and SSLv3 connections. On the server side, this will enable both SSLv3 
and TLSv1_0 connections.
  > 
  > Shouldn't it try with TLS 1.3 when available and fall back to TLS 1.2, but 
not lower (for security reason)?
  
  My opinion is that we should not try to be smarter than Qt here, that 
involves:
  
  - Trusting them on what "SecureProtocols" means (according to 
qsslsocket_openssl.cpp i'd say that SecureProtocols is TlsV1_0OrLater, which 
makes sense to me)
  - Don't do fallbacks "client side" and let openssl do the actual protocol 
negotiation by itself
  
  Besides that, AFAICS from my about:config settings in Firefox, the min 
version it supports is 1 that according to 
https://support.mozilla.org/en-US/questions/1101896 it's TLS 1.0, so supporting 
less than that seems to aggressive to me at this point, yes there may be bugs, 
but we also need to let people use the web.

REPOSITORY
  R241 KIO

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

To: aacid
Cc: jtamate, carewolf, dfaure, stikonas, kde-frameworks-devel, michaelh, 
ngraham, bruns

D16421: Improve emblem contrast, legibility and consistency

[Stefan Brüns]

bruns added a comment.


  In D16421#348960 , @ngraham wrote:
  
  > I wish I could test but I have not gotten the patch to apply so far. Did 
`arc apply D16421` work for you, or did you have to apply it by hand?
  
  
  No, I just copied a few of the icons manually into my installation.
  
  Neither arc patch nor downloading the raw diff and applying it with `patch 
-p1` works.

REPOSITORY
  R266 Breeze Icons

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

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

D16265: [Scheduler] Use flag to track when a runner is going idle

[Stefan Brüns]

This revision was automatically updated to reflect the committed changes.
bruns marked an inline comment as done.
Closed by commit R293:b13453360685: [Scheduler] Use flag to track when a runner 
is going idle (authored by bruns).

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16265?vs=43766=44267

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

AFFECTED FILES
  src/file/fileindexscheduler.cpp
  src/file/fileindexscheduler.h

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

D16441: [KIconLoader] Correct documentation of emblem/overlay order

[Stefan Brüns]

This revision was automatically updated to reflect the committed changes.
Closed by commit R302:030e65f06fed: [KIconLoader] Correct documentation of 
emblem/overlay order (authored by bruns).

REPOSITORY
  R302 KIconThemes

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16441?vs=44263=44265

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

AFFECTED FILES
  src/kiconloader.h

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

D16442: [KIconLoader] Adjust emblem border depending on icon size

[Stefan Brüns]

This revision was automatically updated to reflect the committed changes.
Closed by commit R302:9a94fd6edf07: [KIconLoader] Adjust emblem border 
depending on icon size (authored by bruns).

REPOSITORY
  R302 KIconThemes

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16442?vs=44264=44266

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

AFFECTED FILES
  src/kiconloader.cpp

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

D16442: [KIconLoader] Adjust emblem border depending on icon size

[Nathaniel Graham]

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


  Ah, so that's where those margins were coming from! Big thumbs-up.

REPOSITORY
  R302 KIconThemes

BRANCH
  master

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

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

D16442: [KIconLoader] Adjust emblem border depending on icon size

[Stefan Brüns]

bruns edited the test plan for this revision.

REPOSITORY
  R302 KIconThemes

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

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

D16344: Do not try to fallback to "less secure" protocols

[Andrius Štikonas]

stikonas added a comment.


  Line 89 in 
https://api.kde.org/frameworks/kio/html/ktcpsocket_8cpp_source.html suggests 
that it is the same as QSsl:SecureProtocols.
  
  Can you confirm that it works with TLSv1.2 only sites? (e.g. 
https://stikonas.eu:5281/admin/). Ideally we should  test with TLSv1.3 too.

REPOSITORY
  R241 KIO

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

To: aacid
Cc: jtamate, carewolf, dfaure, stikonas, kde-frameworks-devel, michaelh, 
ngraham, bruns

D16442: [KIconLoader] Adjust emblem border depending on icon size

[Stefan Brüns]

bruns edited the test plan for this revision.

REPOSITORY
  R302 KIconThemes

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

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

D16441: [KIconLoader] Correct documentation of emblem/overlay order

[Nathaniel Graham]

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


  Oops, sorry for missing this with 1c564d0b5eb4859dbcb6d6cbf9d840eca0ea2c56 
!

REPOSITORY
  R302 KIconThemes

BRANCH
  master

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

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

D16442: [KIconLoader] Adjust emblem border depending on icon size

[Stefan Brüns]

bruns created this revision.
bruns added reviewers: Frameworks, VDG, ngraham.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
bruns requested review of this revision.

REVISION SUMMARY
  The current fixed margin of 2px on each side leads to overlapping
  emblems and occlusion of large areas of the main icon.

REPOSITORY
  R302 KIconThemes

BRANCH
  master

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

AFFECTED FILES
  src/kiconloader.cpp

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

D16441: [KIconLoader] Correct documentation of emblem/overlay order

[Stefan Brüns]

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

REVISION SUMMARY
  The order used in the implementation is BR, BL, TL, TR.

REPOSITORY
  R302 KIconThemes

BRANCH
  master

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

AFFECTED FILES
  src/kiconloader.h

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

D16421: Improve emblem contrast, legibility and consistency

[Nathaniel Graham]

ngraham added a comment.


  In D16421#348959 , @bruns wrote:
  
  > Tried to reproduce it here, but for me the bottom-right emblem just renders 
fine here:
  >
  > F6363939: Unpatched_locked_link_48.png 

  
  
  I wish I could test but I have not gotten the patch to apply so far. Did `arc 
apply D16421` work for you, or did you have to apply it by hand?

REPOSITORY
  R266 Breeze Icons

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

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

D16421: Improve emblem contrast, legibility and consistency

[Stefan Brüns]

bruns added a comment.


  In D16421#348609 , @ngraham wrote:
  
  > JFYI, I dug through `KIconLoader` today and couldn't find anything that 
explicitly or implicitly trims the bounds of loaded images that could account 
for the link emblem not having its original bounds respected and results in it 
being drawn too high. Either I missed the part that does this (which is quite 
possible), or it's some implicit thing in the SVG image itself (which is also 
possible, since I know nothing about SVG images), or it's something else 
entirely.
  
  
  Tried to reproduce it here, but for me the bottom-right emblem just renders 
fine here:
  
  F6363939: Unpatched_locked_link_48.png 

REPOSITORY
  R266 Breeze Icons

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

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

D15070: Bindings: Support using sys paths for python install directory

[Stefan Brüns]

bruns added a comment.


  If there are no further comments, I will push this on sunday.

REPOSITORY
  R240 Extra CMake Modules

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

To: bruns, #frameworks
Cc: bcooksley, kde-frameworks-devel, kde-buildsystem, michaelh, ngraham, bruns

D16344: Do not try to fallback to "less secure" protocols

[Jaime Torres Amate]

jtamate added a comment.


  What protocol does KTcpSocket::SecureProtocols implement (I can't guess it)? 
If it is the same as QSsl:SecureProtocols 
  it does:
  On the client side, this will send a TLS 1.0 Client Hello, enabling TLSv1_0 
and SSLv3 connections. On the server side, this will enable both SSLv3 and 
TLSv1_0 connections.
  
  Shouldn't it try with TLS 1.3 when available and fall back to TLS 1.2, but 
not lower (for security reason)?

REPOSITORY
  R241 KIO

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

To: aacid
Cc: jtamate, carewolf, dfaure, stikonas, kde-frameworks-devel, michaelh, 
ngraham, bruns

D16344: Do not try to fallback to "less secure" protocols

[Albert Astals Cid]

aacid added a comment.


  I'll commit this next tuesday unless someone disagrees.

REPOSITORY
  R241 KIO

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

To: aacid
Cc: carewolf, dfaure, stikonas, kde-frameworks-devel, michaelh, ngraham, bruns

D16421: Improve emblem contrast, legibility and consistency

[Noah Davis]

ndavis added a comment.


  In D16421#348866 , @bruns wrote:
  
  > In D16421#348860 , @ndavis wrote:
  >
  > > The other issue with using a font based question mark is I also have to 
use that for the 8px icon if I want consistency.
  >
  >
  > There is nothing wrong with using the font outline as a starting point and 
tweaking it for pixel grid alignment manually.
  >
  > > Is a 6pt Noto Sans question mark legible enough?
  > > 
  > > normal 6pt Noto Sans F6363258: Screenshot_20181026_073539.png 

  > > 
  > > Bold 6pt Noto Sans F6363273: Screenshot_20181026_074013.png 

  >
  > I think the bold one is.
  >
  > > drawn F6363264: Screenshot_20181026_073837.png 

  >
  > The hand drawn is actually 7px high ...
  
  
  Yeah, I cheated on the drawn one because I'd have to come up with a brand new 
style to make it fit in a 6px area. I've also made the exclaimation marks and 
'i's of the 8px emblems 7px high to improve the ratio of dot to body.

REPOSITORY
  R266 Breeze Icons

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

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

D16421: Improve emblem contrast, legibility and consistency

[Stefan Brüns]

bruns added a comment.


  In D16421#348860 , @ndavis wrote:
  
  > The other issue with using a font based question mark is I also have to use 
that for the 8px icon if I want consistency.
  
  
  There is nothing wrong with using the font outline as a starting point and 
tweaking it for pixel grid alignment manually.
  
  > Is a 6pt Noto Sans question mark legible enough?
  > 
  > normal 6pt Noto Sans F6363258: Screenshot_20181026_073539.png 

  > 
  > Bold 6pt Noto Sans F6363273: Screenshot_20181026_074013.png 

  
  I think the bold one is.
  
  > drawn F6363264: Screenshot_20181026_073837.png 

  
  The hand drawn is actually 7px high ...

REPOSITORY
  R266 Breeze Icons

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

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

D16421: Improve emblem contrast, legibility and consistency

[Noah Davis]

ndavis added a comment.


  In D16421#348857 , @bruns wrote:
  
  > In D16421#348826 , @ndavis wrote:
  >
  > > I could, but then I'm running into this issue: T9898 

  > >
  > > I made that task when I was trying to come up with a more legible style 
for emblem-question. I chose this style for consistency, but there is already a 
consistency problem.
  >
  >
  > Fair point.
  >  The used fonts seem to be (for the question marks):
  >
  > - Noto Sans or Droid Sans, but rectangular dot?
  > - 2x homegrown
  > - 2x Nimbus Sans Bold
  > - Oxygen Sans
  
  
  The other issue with using a font based question mark is I also have to use 
that for the 8px icon if I want consistency.
  
  Is a 6pt Noto Sans question mark legible enough? F6363258: 
Screenshot_20181026_073539.png 

REPOSITORY
  R266 Breeze Icons

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

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

D16421: Improve emblem contrast, legibility and consistency

[Stefan Brüns]

bruns added a comment.


  In D16421#348826 , @ndavis wrote:
  
  > I could, but then I'm running into this issue: T9898 

  >
  > I made that task when I was trying to come up with a more legible style for 
emblem-question. I chose this style for consistency, but there is already a 
consistency problem.
  
  
  Fair point.
  The used fonts seem to be (for the question marks):
  
  - Noto Sans or Droid Sans, but rectangular dot?
  - 2x homegrown
  - 2x Nimbus Sans Bold
  - Oxygen Sans

REPOSITORY
  R266 Breeze Icons

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

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

D16421: Improve emblem contrast, legibility and consistency

[Noah Davis]

ndavis added a comment.


  Slight change to make the 16 and 22 px icons look more like the 8px icon:
  F6363210: Screenshot_20181026_072203.png 


REPOSITORY
  R266 Breeze Icons

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

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

D16421: Improve emblem contrast, legibility and consistency

[Noah Davis]

ndavis updated this revision to Diff 44248.
ndavis added a comment.


  Widen the circular portion of 16/22 px emblem-question icons

REPOSITORY
  R266 Breeze Icons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16421?vs=44230=44248

BRANCH
  emblem-outlines (branched from master)

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

AFFECTED FILES
  icons-dark/emblems/16/checkmark.svg
  icons-dark/emblems/16/emblem-added.svg
  icons-dark/emblems/16/emblem-checked.svg
  icons-dark/emblems/16/emblem-encrypted-unlocked.svg
  icons-dark/emblems/16/emblem-error.svg
  icons-dark/emblems/16/emblem-favorite.svg
  icons-dark/emblems/16/emblem-important.svg
  icons-dark/emblems/16/emblem-information.svg
  icons-dark/emblems/16/emblem-locked.svg
  icons-dark/emblems/16/emblem-mounted.svg
  icons-dark/emblems/16/emblem-pause.svg
  icons-dark/emblems/16/emblem-question.svg
  icons-dark/emblems/16/emblem-remove.svg
  icons-dark/emblems/16/emblem-shared.svg
  icons-dark/emblems/16/emblem-success.svg
  icons-dark/emblems/16/emblem-symbolic-link.svg
  icons-dark/emblems/16/emblem-unavailable.svg
  icons-dark/emblems/16/emblem-unlocked.svg
  icons-dark/emblems/16/emblem-unmounted.svg
  icons-dark/emblems/16/emblem-warning.svg
  icons-dark/emblems/16/rating-unrated.svg
  icons-dark/emblems/16/rating.svg
  icons-dark/emblems/16/vcs-added.svg
  icons-dark/emblems/16/vcs-conflicting.svg
  icons-dark/emblems/16/vcs-locally-modified-unstaged.svg
  icons-dark/emblems/16/vcs-locally-modified.svg
  icons-dark/emblems/16/vcs-normal.svg
  icons-dark/emblems/16/vcs-removed.svg
  icons-dark/emblems/16/vcs-update-required.svg
  icons-dark/emblems/22/checkmark.svg
  icons-dark/emblems/22/emblem-added.svg
  icons-dark/emblems/22/emblem-checked.svg
  icons-dark/emblems/22/emblem-encrypted-unlocked.svg
  icons-dark/emblems/22/emblem-error.svg
  icons-dark/emblems/22/emblem-favorite.svg
  icons-dark/emblems/22/emblem-important.svg
  icons-dark/emblems/22/emblem-information.svg
  icons-dark/emblems/22/emblem-locked.svg
  icons-dark/emblems/22/emblem-mounted.svg
  icons-dark/emblems/22/emblem-pause.svg
  icons-dark/emblems/22/emblem-question.svg
  icons-dark/emblems/22/emblem-remove.svg
  icons-dark/emblems/22/emblem-shared.svg
  icons-dark/emblems/22/emblem-success.svg
  icons-dark/emblems/22/emblem-symbolic-link.svg
  icons-dark/emblems/22/emblem-unavailable.svg
  icons-dark/emblems/22/emblem-unlocked.svg
  icons-dark/emblems/22/emblem-unmounted.svg
  icons-dark/emblems/22/emblem-warning.svg
  icons-dark/emblems/22/rating-unrated.svg
  icons-dark/emblems/22/rating.svg
  icons-dark/emblems/22/vcs-added.svg
  icons-dark/emblems/22/vcs-conflicting.svg
  icons-dark/emblems/22/vcs-locally-modified-unstaged.svg
  icons-dark/emblems/22/vcs-locally-modified.svg
  icons-dark/emblems/22/vcs-normal.svg
  icons-dark/emblems/22/vcs-removed.svg
  icons-dark/emblems/22/vcs-update-required.svg
  icons-dark/emblems/8/emblem-added.svg
  icons-dark/emblems/8/emblem-checked.svg
  icons-dark/emblems/8/emblem-encrypted-unlocked.svg
  icons-dark/emblems/8/emblem-error.svg
  icons-dark/emblems/8/emblem-favorite.svg
  icons-dark/emblems/8/emblem-important.svg
  icons-dark/emblems/8/emblem-information.svg
  icons-dark/emblems/8/emblem-locked.svg
  icons-dark/emblems/8/emblem-mounted.svg
  icons-dark/emblems/8/emblem-pause.svg
  icons-dark/emblems/8/emblem-question.svg
  icons-dark/emblems/8/emblem-remove.svg
  icons-dark/emblems/8/emblem-shared.svg
  icons-dark/emblems/8/emblem-symbolic-link.svg
  icons-dark/emblems/8/emblem-unavailable.svg
  icons-dark/emblems/8/emblem-unlocked.svg
  icons-dark/emblems/8/emblem-unmounted.svg
  icons-dark/emblems/8/emblem-warning.svg
  icons-dark/emblems/8/rating-unrated.svg
  icons-dark/emblems/8/rating.svg
  icons-dark/emblems/8/vcs-added.svg
  icons-dark/emblems/8/vcs-conflicting.svg
  icons-dark/emblems/8/vcs-locally-modified-unstaged.svg
  icons-dark/emblems/8/vcs-locally-modified.svg
  icons-dark/emblems/8/vcs-normal.svg
  icons-dark/emblems/8/vcs-removed.svg
  icons-dark/emblems/8/vcs-update-required.svg
  icons/emblems/16/checkmark.svg
  icons/emblems/16/emblem-added.svg
  icons/emblems/16/emblem-checked.svg
  icons/emblems/16/emblem-encrypted-unlocked.svg
  icons/emblems/16/emblem-error.svg
  icons/emblems/16/emblem-favorite.svg
  icons/emblems/16/emblem-important.svg
  icons/emblems/16/emblem-information.svg
  icons/emblems/16/emblem-locked.svg
  icons/emblems/16/emblem-mounted.svg
  icons/emblems/16/emblem-pause.svg
  icons/emblems/16/emblem-question.svg
  icons/emblems/16/emblem-remove.svg
  icons/emblems/16/emblem-shared.svg
  icons/emblems/16/emblem-success.svg
  icons/emblems/16/emblem-symbolic-link.svg
  icons/emblems/16/emblem-unavailable.svg
  icons/emblems/16/emblem-unlocked.svg
  icons/emblems/16/emblem-unmounted.svg
  icons/emblems/16/emblem-warning.svg
  icons/emblems/16/rating-unrated.svg
  icons/emblems/16/rating.svg
  icons/emblems/16/vcs-added.svg
  icons/emblems/16/vcs-conflicting.svg
  

D16434: Fix keyboard layout change notifications

[Fabian Vogt]

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


  Don't use a union. Still works.

REPOSITORY
  R268 KGlobalAccel

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16434?vs=44241=44247

BRANCH
  master

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

AFFECTED FILES
  CMakeLists.txt
  src/runtime/plugins/CMakeLists.txt
  src/runtime/plugins/xcb/CMakeLists.txt
  src/runtime/plugins/xcb/kglobalaccel_x11.cpp
  src/runtime/plugins/xcb/kglobalaccel_x11.h

To: fvogt, #frameworks, #plasma
Cc: anthonyfieroni, kde-frameworks-devel, michaelh, ngraham, bruns

D16434: Fix keyboard layout change notifications

[Fabian Vogt]

fvogt marked 4 inline comments as done.

REPOSITORY
  R268 KGlobalAccel

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

To: fvogt, #frameworks, #plasma
Cc: anthonyfieroni, kde-frameworks-devel, michaelh, ngraham, bruns

D16434: Fix keyboard layout change notifications

[Fabian Vogt]

fvogt added a comment.


  Note that the way it's done is ~~copied from~~ inspired by QXcbConnection.
  
  I don't like it either though, so I'll try getting rid of the union.

REPOSITORY
  R268 KGlobalAccel

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

To: fvogt, #frameworks, #plasma
Cc: anthonyfieroni, kde-frameworks-devel, michaelh, ngraham, bruns

D16434: Fix keyboard layout change notifications

[Anthony Fieroni]

anthonyfieroni added inline comments.

INLINE COMMENTS

> kglobalaccel_x11.cpp:198-208
> +typedef union {
> + /* All XKB events share these fields. */
> + struct {
> + uint8_t response_type;
> + uint8_t xkbType;
> + uint16_t sequence;
> + xcb_timestamp_t time;

I see what you doing, but don't do it. See below.

> kglobalaccel_x11.cpp:233
> +if(m_xkb_first_event && responseType == m_xkb_first_event) {
> +_xkb_event *xkb_event = reinterpret_cast<_xkb_event*>(event);
> +switch (xkb_event->any.xkbType) {

Cast to xcb_generic_event_t

> kglobalaccel_x11.cpp:234
> +_xkb_event *xkb_event = reinterpret_cast<_xkb_event*>(event);
> +switch (xkb_event->any.xkbType) {
> +case XCB_XKB_MAP_NOTIFY:

Use pad0 (stupid name but you can get it as ref and name as you want)

> kglobalaccel_x11.cpp:239
> +case XCB_XKB_NEW_KEYBOARD_NOTIFY: {
> +xcb_xkb_new_keyboard_notify_event_t *ev = 
> _event->new_keyboard_notify;
> +if (ev->changed & XCB_XKB_NKN_DETAIL_KEYCODES)

Cast event to xcb_xkb_new_keyboard_notify_event_t.

REPOSITORY
  R268 KGlobalAccel

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

To: fvogt, #frameworks, #plasma
Cc: anthonyfieroni, kde-frameworks-devel, michaelh, ngraham, bruns

D16421: Improve emblem contrast, legibility and consistency

[Noah Davis]

ndavis added a comment.


  In D16421#348825 , @bruns wrote:
  
  > In D16421#348664 , @ndavis wrote:
  >
  > > In D16421#348594 , @ngraham 
wrote:
  > >
  > > > Now that I stare at the summary graphics again, the white question mark 
looks a bit wispy and insubstantial at the 16px and 22px sizes. Do you agree? 
Other than that, everything looks pretty much perfect to me.
  > >
  > >
  > > How is this?
  > >  F6356555: Screenshot_20181025_193720.png 

  >
  >
  > Much better (weight matches), but I don't like the sharp corner in the 
center to much. Can you try using the question mark from "Noto Sans"?
  
  
  I could, but then I'm running into this issue: 
https://phabricator.kde.org/T9898
  
  I made that task when I was trying to come up with a more legible style for 
emblem-question. I chose this style for consistency, but there is already a 
consistency problem.

REPOSITORY
  R266 Breeze Icons

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

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

D16421: Improve emblem contrast, legibility and consistency

[Stefan Brüns]

bruns added a comment.


  In D16421#348664 , @ndavis wrote:
  
  > In D16421#348594 , @ngraham 
wrote:
  >
  > > Now that I stare at the summary graphics again, the white question mark 
looks a bit wispy and insubstantial at the 16px and 22px sizes. Do you agree? 
Other than that, everything looks pretty much perfect to me.
  >
  >
  > How is this?
  >  F6356555: Screenshot_20181025_193720.png 

  
  
  Much better (weight matches), but I don't like the sharp corner in the center 
to much. Can you try using the question mark from "Noto Sans"?

REPOSITORY
  R266 Breeze Icons

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

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

D16434: Fix keyboard layout change notifications

[Fabian Vogt]

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

REVISION SUMMARY
  This rework fixes several issues:
  
  - Qt wasn't informed about XCB_MAPPING_NOTIFY anymore
  - With XKB enabled in Qt, X won't send XCB_MAPPING_NOTIFY anymore. So listen 
for XKB events as well.
  - Install the event filter before fetching the keysym mapping to close a race 
window
  - Use the old mapping for ungrabbing
  
  BUG: 350816
  BUG: 269403

TEST PLAN
  Ctrl-Alt-Y global shortcut works even when doing
  "setxkbmap us; kglobalaccel5 & sleep 1; setxkbmap de;" while it did not
  before. On some systems, this race happened on every login, now it works
  reliably.

REPOSITORY
  R268 KGlobalAccel

BRANCH
  master

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

AFFECTED FILES
  CMakeLists.txt
  src/runtime/plugins/CMakeLists.txt
  src/runtime/plugins/xcb/CMakeLists.txt
  src/runtime/plugins/xcb/kglobalaccel_x11.cpp
  src/runtime/plugins/xcb/kglobalaccel_x11.h

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