D28353: Changed contrast effect values to have more transparency, and then changed transparency accordingly

[Noah Davis]

ndavis added a comment.


  I generally prefer type 1

REPOSITORY
  R242 Plasma Framework (Library)

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

To: niccolove, #vdg, #plasma, cblack
Cc: ndavis, filipf, ngraham, cblack, kde-frameworks-devel, LeGast00n, michaelh, 
bruns

D29420: Generate DBus interface

[Nicolas Fella]

nicolasfella updated this revision to Diff 82237.
nicolasfella added a comment.


  - Use q as parent
  - Change reply type
  - Use ranged-for

REPOSITORY
  R289 KNotifications

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29420?vs=81920&id=82237

BRANCH
  geninterface

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

AFFECTED FILES
  src/notifybypopup.cpp
  src/notifybypopup.h

To: nicolasfella, #frameworks, broulik
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29420: Generate DBus interface

[Nicolas Fella]

nicolasfella marked 3 inline comments as done.

REPOSITORY
  R289 KNotifications

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

To: nicolasfella, #frameworks, broulik
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29513: [kcm trash] Change kcm trash size percent to 2 decimal places

[Nathaniel Graham]

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

REPOSITORY
  R241 KIO

BRANCH
  trash

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

To: shubham, ngraham
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28353: Changed contrast effect values to have more transparency, and then changed transparency accordingly

[Nathaniel Graham]

ngraham added a comment.


  I like #2 the best!

REPOSITORY
  R242 Plasma Framework (Library)

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

To: niccolove, #vdg, #plasma, cblack
Cc: filipf, ngraham, cblack, kde-frameworks-devel, LeGast00n, michaelh, bruns

D28353: Changed contrast effect values to have more transparency, and then changed transparency accordingly

[Niccolò Venerandi]

niccolove edited the test plan for this revision.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: niccolove, #vdg, #plasma, cblack
Cc: filipf, ngraham, cblack, kde-frameworks-devel, LeGast00n, michaelh, bruns

D28353: Changed contrast effect values to have more transparency, and then changed transparency accordingly

[Niccolò Venerandi]

niccolove edited the test plan for this revision.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: niccolove, #vdg, #plasma, cblack
Cc: filipf, ngraham, cblack, kde-frameworks-devel, LeGast00n, michaelh, bruns

D28353: Changed contrast effect values to have more transparency, and then changed transparency accordingly

[Niccolò Venerandi]

niccolove edited the test plan for this revision.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: niccolove, #vdg, #plasma, cblack
Cc: filipf, ngraham, cblack, kde-frameworks-devel, LeGast00n, michaelh, bruns

D28353: Changed contrast effect values to have more transparency, and then changed transparency accordingly

[Niccolò Venerandi]

niccolove edited the test plan for this revision.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: niccolove, #vdg, #plasma, cblack
Cc: filipf, ngraham, cblack, kde-frameworks-devel, LeGast00n, michaelh, bruns

D25339: update lineHeight if boundingRect indicates a larger value.

[René J . V . Bertin]

rjvbb added a comment.


  > the code can be smart
  
  No, cleverly written at best (don't get me started! ;) )
  
  > but it can't know how a user prefers to read text, there is no one-size 
fits all.
  
  EXACTLY. Words becomes a lot more readable for me when I use 64pt or larger 
so I read from my recliner ... but given the size of my screens it would texts 
less readable. IOW, there's a trade-off there and that also applies to 
lineheight, and esp. with lineheight it also depends on the context. Many forms 
of code are easier to work with with a minimal lineheight because that makes it 
easier to see code organisation (blocks/scopes identified by indentation). So 
yeah, a user-controllable scale factor could be a good idea, independent from 
the issue at hand.
  
  I didn't mention Konsole because AFAIK it doesn't use KTextEditor.

REPOSITORY
  R39 KTextEditor

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

To: xuetianweng, #ktexteditor, cullmann, dhaumann, #frameworks, rjvbb
Cc: ahmadsamir, brauch, sars, pshinjo, rjvbb, fakefred, anthonyfieroni, 
kde-frameworks-devel, kwrite-devel, rrosch, LeGast00n, cblack, domson, 
michaelh, ngraham, bruns, demsking, cullmann, dhaumann

D29123: Do not mark entry as uninstalled if uninstallation script failed

[Alexander Lohnau]

alex updated this revision to Diff 82228.
alex added a comment.


  Update docs
  
  I adjusted your suggestion a bit, the signalEntryChanged gets also emitted, 
when
  the user cancels the uninstallation :-).

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29123?vs=82114&id=82228

BRANCH
  arcpatch-D29123_1

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

AFFECTED FILES
  src/core/engine.cpp
  src/core/installation.cpp
  src/core/installation.h

To: alex, #knewstuff, meven, ngraham, leinir
Cc: leinir, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29342: Implement support for notification urgency on Android

[Volker Krause]

This revision was automatically updated to reflect the committed changes.
Closed by commit R289:9a13dd26d1de: Implement support for notification urgency 
on Android (authored by vkrause).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D29342?vs=81695&id=82227#toc

REPOSITORY
  R289 KNotifications

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29342?vs=81695&id=82227

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

AFFECTED FILES
  src/android/org/kde/knotifications/KNotification.java
  src/android/org/kde/knotifications/NotifyByAndroid.java
  src/notifybyandroid.cpp

To: vkrause, nicolasfella
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28208: Move sni icon handling logic from data engine to applet

[Konrad Materka]

kmaterka added a comment.


  @davidre can you extract your test improvement to a separate patch? it would 
help me in another issue and I don't want to "steal" your code :)

REPOSITORY
  R120 Plasma Workspace

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

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

D25339: update lineHeight if boundingRect indicates a larger value.

[Ahmad Samir]

ahmadsamir added a comment.


  In D25339#665827 , @xuetianweng 
wrote:
  
  >
  
  
  [...]
  
  > As for higher line, it might not as bad as you thought as it actually might 
improve readability for many people.
  
  I agree with this statement :). Thanks to this diff I found out where the 
line height can be manipulated; the way the code computed it, fontHeight was 34 
(IIRC), I've hardcoded it to 40 and I very much prefer reading text that way.
  
  Note that Konsole tries and compute a sane line height to accommodate CJK 
characters... etc, but it also has a config option to change the line height 
(Settings -> Appearance -> Miscellaneous)... the code can be smart, but it 
can't know how a user prefers to read text, there is no one-size fits all.

REPOSITORY
  R39 KTextEditor

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

To: xuetianweng, #ktexteditor, cullmann, dhaumann, #frameworks, rjvbb
Cc: ahmadsamir, brauch, sars, pshinjo, rjvbb, fakefred, anthonyfieroni, 
kde-frameworks-devel, kwrite-devel, rrosch, LeGast00n, cblack, domson, 
michaelh, ngraham, bruns, demsking, cullmann, dhaumann

D25339: update lineHeight if boundingRect indicates a larger value.

[Xuetian Weng]

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


  In D25339#665432 , @rjvbb wrote:
  
  > With "we've ever seen" you do mean that lineheight only changes when a line 
that requires it scrolls into view?
  >
  > >   Though line height won't shrink during the edit phase, it will back to 
the actual value upon save.
  >
  > Have you tried to reset the max. lineheight on each redraw (I presume the 
scrollbars could give you a signal that a view scroll/jump was initiated, if 
need be). 
  >  Something tells me that it'd be nicer if lineheight always is as small as 
possible. Imagine you're using a smallish window to edit a document that just 
has some of these "offending", much higher characters at the bottom. If it gets 
into view only once, lineheight increases and it's as if you lose a lot of 
screen estate until you save the document. Now I mustn't be the only one who 
often doesn't save for a while, esp. when doing things like moving blocks of 
text around, and it's exactly in that kind of scenario where having to save to 
get a more space-efficient rendering back can be very annoying.
  >
  > As long as you can determine the max. lineheight required for the view 
that's about to be drawn before the view is actually drawn there should be no 
glitches. You'd just see a jump in lineheight and there would probably be an 
interesting problem to tackle with edge cases where the higher glyphs fall 
outside the view area because of the increased lineheight, but that's something 
your current implementation cannot avoid completely either. As to the changing 
lineheight: I think users will understand why it happens and get used to it. 
It's comparable to what you see in graphics programmes that show cursor 
co-ordinates next to the cursor; that indicator has to jump when it would get 
"pushed out" of the view, and that doesn't even feel weird.
  >
  > I presume your new approach would work not just for CJK characters, but 
also for anything else that changes the lineheight. That's and advantage but 
could also lead to regressions for some (who never use CJK characters or who, 
like me, wouldn't care how they display because they can't read them anyway). 
Emoticons come to mind; here too I don't really care if they get cut off. Scrap 
that, I *really* don't care if they are...
  
  
  To be honest,  I didn't see issue about color emoji (until select them). My 
"fill" gap code seems can't make color emoji bitmap transparent The fill 
gap code is indeed a hack.
  
  Probably when I get time I might try to make it able to render view with 
different height...
  
  As for higher line, it might not as bad as you thought as it actually might 
improve readability for many people.

REPOSITORY
  R39 KTextEditor

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

To: xuetianweng, #ktexteditor, cullmann, dhaumann, #frameworks, rjvbb
Cc: brauch, sars, pshinjo, rjvbb, fakefred, anthonyfieroni, 
kde-frameworks-devel, kwrite-devel, rrosch, LeGast00n, cblack, domson, 
michaelh, ngraham, bruns, demsking, cullmann, dhaumann

D28353: Changed contrast effect values to have more transparency, and then changed transparency accordingly

[Niccolò Venerandi]

niccolove edited the test plan for this revision.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: niccolove, #vdg, #plasma, cblack
Cc: filipf, ngraham, cblack, kde-frameworks-devel, LeGast00n, michaelh, bruns

D29513: [kcm trash] Change kcm trash size percent to 2 decimal places

[Shubham]

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

REVISION SUMMARY
  This patch changes the precision from 3 decimal places to 2

TEST PLAN
  Go to dolphin->Settings->Configure Dolhin->Trash

REPOSITORY
  R241 KIO

BRANCH
  trash

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

AFFECTED FILES
  src/ioslaves/trash/kcmtrash.cpp

To: shubham, ngraham
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

[Gustavo Carneiro]

araujoluis updated this revision to Diff 82219.
araujoluis added a comment.


  - kwidgetsaddons: kcolorcombo: code refactoring

REPOSITORY
  R236 KWidgetsAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29502?vs=82200&id=82219

BRANCH
  named_color_support

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

AFFECTED FILES
  src/kcolorcombo.cpp
  src/kcolorcombo.h
  tests/kcolorcombotest.cpp

To: araujoluis, tcanabrava, patrickelectric, hindenburg, ngraham
Cc: broulik, cfeck, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, 
bruns

D29511: Label: Add ping-pong logic

[patrick j pereira]

patrickelectric added a comment.


  Thanks @davidedmundson and @ognarb, I was able to replicate the issue, I'll 
think a bit more in my approach

REPOSITORY
  R242 Plasma Framework (Library)

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

To: patrickelectric, #plasma, #vdg, ognarb, davidedmundson
Cc: davidedmundson, ognarb, cblack, kde-frameworks-devel, LeGast00n, michaelh, 
ngraham, bruns

D29511: Label: Add ping-pong logic

[David Edmundson]

davidedmundson requested changes to this revision.
davidedmundson added a comment.


  Plasmacomponents is deprecated please see Plasmacomponents3 then see the 
readme within that and put this somewhere else.
  
  1. Label is a super super super common element we don't want to add any 
overhead instantiating a label. A new special class would be best
  2. It can all be done in one SequentialAnimation, you don't need states.
  3. You don't need to get the width again, we know this already from the other 
text properties.
  4. This is broken for any text that's wrapped or elided

REPOSITORY
  R242 Plasma Framework (Library)

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

To: patrickelectric, #plasma, #vdg, ognarb, davidedmundson
Cc: davidedmundson, ognarb, cblack, kde-frameworks-devel, LeGast00n, michaelh, 
ngraham, bruns

D29511: Label: Add ping-pong logic

[Carl Schwan]

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


  I'm not sure it is a good idea, but at least for me it break the clock in my 
panel.
  
  F8294594: vokoscreenNG-2020-05-07_17-49-49.mkv 


REPOSITORY
  R242 Plasma Framework (Library)

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

To: patrickelectric, #plasma, #vdg, ognarb
Cc: ognarb, cblack, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D29511: Label: Add ping-pong logic

[patrick j pereira]

patrickelectric added a comment.


  Outoput:
  F8294587: deepin-screen-recorder_qmlscene_20200507124715.gif 


REPOSITORY
  R242 Plasma Framework (Library)

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

To: patrickelectric, #plasma, #vdg
Cc: cblack, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D29511: Label: Add ping-pong logic

[Carson Black]

cblack added a comment.


  You should include a video in the test plan since you're creating visual 
changes with motion.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: patrickelectric, #plasma, #vdg
Cc: cblack, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D29511: Label: Add ping-pong logic

[patrick j pereira]

patrickelectric updated this revision to Diff 82218.
patrickelectric added a comment.


  Update style

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29511?vs=82217&id=82218

BRANCH
  ping_pong_text

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

AFFECTED FILES
  src/declarativeimports/plasmacomponents/qml/Label.qml

To: patrickelectric
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29511: Label: Add ping-pong logic

[Carson Black]

cblack added reviewers: Plasma, VDG.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: patrickelectric, #plasma, #vdg
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29511: Label: Add ping-pong logic

[patrick j pereira]

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

REVISION SUMMARY
  Add friendly ping-pong animation to allow visibility of full text
  in an animated way
  
  Signed-off-by: Patrick José Pereira 

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  ping_pong_text

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

AFFECTED FILES
  src/declarativeimports/plasmacomponents/qml/Label.qml

To: patrickelectric
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

[Tomaz Canabrava]

tcanabrava added inline comments.

INLINE COMMENTS

> kcolorcombo.cpp:244
> +{
> +QList namedColors;
> +for (int colorIndex = 0; colorIndex < colors.count(); colorIndex++) {

namedColors.reserve(colors.size());

> kcolorcombo.cpp:245
> +QList namedColors;
> +for (int colorIndex = 0; colorIndex < colors.count(); colorIndex++) {
> +namedColors.insert(colorIndex, { QString(), colors[colorIndex] });

for(auto color : colors)

> kcolorcombo.cpp:288
> +for (int index = 0; index < STANDARD_PALETTE_SIZE; ++index) {
> +//list += {QString(), standardColor(index)};
> +}

why is there a for running with all code comented out?

> kcolorcombo.cpp:291
> +return list;
>  } else {
>  return d->colorList;

remove the else, just return directly.

> araujoluis wrote in kcolorcombo.h:60
> broulik try a solution in several ways, but it looks like this was the most 
> convenient one found so far.

QPair - no need for a struct.

REPOSITORY
  R236 KWidgetsAddons

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

To: araujoluis, tcanabrava, patrickelectric, hindenburg, ngraham
Cc: broulik, cfeck, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, 
bruns

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

[Gustavo Carneiro]

araujoluis added a comment.


  In D29502#665600 , @tcanabrava 
wrote:
  
  > In D29502#665582 , @cfeck wrote:
  >
  > > Does the delegate ensure the text is rendered in a color visible over the 
colored background?
  >
  >
  > not yet, I talked to gustavo and he's working in an updated version of the 
patch.
  >
  > This is the current - not in the diff yet - version:
  >  F8293330: image.png 
  
  
  Tomaz the new patch is now available for review

INLINE COMMENTS

> broulik wrote in kcolorcombo.h:60
> Do we really want to leak this `struct` into public API?

broulik try a solution in several ways, but it looks like this was the most 
convenient one found so far.

REPOSITORY
  R236 KWidgetsAddons

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

To: araujoluis, tcanabrava, patrickelectric, hindenburg, ngraham
Cc: broulik, cfeck, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, 
bruns

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

[Gustavo Carneiro]

araujoluis edited the summary of this revision.

REPOSITORY
  R236 KWidgetsAddons

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

To: araujoluis, tcanabrava, patrickelectric, hindenburg, ngraham
Cc: broulik, cfeck, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, 
bruns

D29503: Pixel align children of GridViewInternal

[Fabian Vogt]

fvogt added a comment.


  In D29503#665612 , @ngraham wrote:
  
  > I can't reproduce it, but I wonder if this could fix or help 
https://bugs.kde.org/show_bug.cgi?id=417488?
  
  
  Yup, that's exactly what I was seeing before this change: F8293538: 
8sMM0Gq.png  vs F8293533: 
Screenshot_20200507_153104.png 

REPOSITORY
  R296 KDeclarative

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

To: fvogt, #frameworks, broulik, mart, davidedmundson
Cc: ngraham, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

[Gustavo Carneiro]

araujoluis updated this revision to Diff 82200.
araujoluis added a comment.


  - kwidgetsaddons: kcolorcombo: fix colors() out of range
  - kwidgetsaddons: kcolorcombo: tests: add example for use named colors.
  - kwidgetsaddons: kcolorcombo: update named colow view.

REPOSITORY
  R236 KWidgetsAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29502?vs=82179&id=82200

BRANCH
  named_color_support

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

AFFECTED FILES
  src/kcolorcombo.cpp
  src/kcolorcombo.h
  tests/kcolorcombotest.cpp

To: araujoluis, tcanabrava, patrickelectric, hindenburg, ngraham
Cc: broulik, cfeck, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, 
bruns

D29503: Pixel align children of GridViewInternal

[Nathaniel Graham]

ngraham added a comment.


  I can't reproduce it, but I wonder if this could fix or help 
https://bugs.kde.org/show_bug.cgi?id=417488?

REPOSITORY
  R296 KDeclarative

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

To: fvogt, #frameworks, broulik, mart, davidedmundson
Cc: ngraham, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns

D29123: Do not mark entry as uninstalled if uninstallation script failed

[Nathaniel Graham]

ngraham accepted this revision.

REPOSITORY
  R304 KNewStuff

BRANCH
  arcpatch-D29123_1

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

To: alex, #knewstuff, meven, ngraham, leinir
Cc: leinir, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> kcolorcombo.h:60
> +/** Struct used in NamedColors list */
> +struct KNamedColor
> +{

Do we really want to leak this `struct` into public API?

REPOSITORY
  R236 KWidgetsAddons

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

To: araujoluis, tcanabrava, patrickelectric, hindenburg, ngraham
Cc: broulik, cfeck, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, 
bruns

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

[Tomaz Canabrava]

tcanabrava added a comment.


  In D29502#665582 , @cfeck wrote:
  
  > Does the delegate ensure the text is rendered in a color visible over the 
colored background?
  
  
  not yet, I talked to gustavo and he's working in an updated version of the 
patch.
  
  This is the current - not in the diff yet - version:
  F8293330: image.png 

REPOSITORY
  R236 KWidgetsAddons

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

To: araujoluis, tcanabrava, patrickelectric, hindenburg, ngraham
Cc: cfeck, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D25339: update lineHeight if boundingRect indicates a larger value.

[René J . V . Bertin]

rjvbb added a comment.


  This new version does not cause a lineheight regression for me (after 
backporting it to 5.60.0). However, contrary to what I thought it does not 
react to emoji
  
  F8293110: emoji.txt 

REPOSITORY
  R39 KTextEditor

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

To: xuetianweng, #ktexteditor, cullmann, dhaumann, #frameworks, rjvbb
Cc: brauch, sars, pshinjo, rjvbb, fakefred, anthonyfieroni, 
kde-frameworks-devel, kwrite-devel, rrosch, LeGast00n, cblack, domson, 
michaelh, ngraham, bruns, demsking, cullmann, dhaumann

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

[Christoph Feck]

cfeck added a comment.


  Does the delegate ensure the text is rendered in a color visible over the 
colored background?

REPOSITORY
  R236 KWidgetsAddons

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

To: araujoluis, tcanabrava, patrickelectric, hindenburg, ngraham
Cc: cfeck, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29175: DBus Runner: Add service property to request actions once

[Méven Car]

meven added inline comments.

INLINE COMMENTS

> alex wrote in dbusrunnertest.cpp:206
> The concept behind this test is, that the runner is just loaded. If the 
> Request-Actions-Once property is correctly implemented the actions are 
> requested when the plugin is initialized. That means that they should be 
> available and `prepare` doesn't need to be called.
> 
> So it would make sense to add a signal spy to check if prepare was not called 
> at all, or am I missing something?

I am not sure the test illustrates the change.
1 action available means it did not regress but you don't test the actual new 
behavior AFAICT

Only by adding signals spies can you test the actual behavior here.

I would trigger a second match and check requestActions was only called once, 
but prepare called each time as usual.

REPOSITORY
  R308 KRunner

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

To: alex, #plasma, meven, ngraham, broulik
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29455: KNS: Deprecate isRemote method and handle parse error properly

[Alexander Lohnau]

This revision was automatically updated to reflect the committed changes.
Closed by commit R304:9575f2defb27: KNS: Deprecate isRemote method and handle 
parse error properly (authored by alex).

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29455?vs=82098&id=82190

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

AFFECTED FILES
  src/core/CMakeLists.txt
  src/core/engine.cpp
  src/core/installation.cpp
  src/core/installation.h

To: alex, #knewstuff, ngraham, leinir
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29451: KNS: Do not mark entry as installed if install script failed

[Alexander Lohnau]

This revision was automatically updated to reflect the committed changes.
Closed by commit R304:2838e55acd17: KNS: Do not mark entry as installed if 
install script failed (authored by alex).

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29451?vs=82180&id=82189

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

AFFECTED FILES
  src/core/installation.cpp

To: alex, #knewstuff, ngraham, leinir
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

KDE CI: Frameworks » kdeclarative » kf5-qt5 FreeBSDQt5.14 - Build # 12 - Still Unstable!

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks/job/kdeclarative/job/kf5-qt5%20FreeBSDQt5.14/12/
 Project:
kf5-qt5 FreeBSDQt5.14
 Date of build:
Thu, 07 May 2020 09:59:21 +
 Build duration:
1 min 45 sec and counting
   JUnit Tests
  Name: projectroot Failed: 1 test(s), Passed: 1 test(s), Skipped: 0 test(s), Total: 2 test(s)Failed: projectroot.autotests.quickviewsharedengine

D29503: Pixel align children of GridViewInternal

[Fabian Vogt]

This revision was automatically updated to reflect the committed changes.
Closed by commit R296:9725a21bcd0e: Pixel align children of GridViewInternal 
(authored by fvogt).

REPOSITORY
  R296 KDeclarative

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29503?vs=82183&id=82184

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

AFFECTED FILES
  src/qmlcontrols/kcmcontrols/qml/private/GridViewInternal.qml

To: fvogt, #frameworks, broulik, mart, davidedmundson
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29455: KNS: Deprecate isRemote method and handle parse error properly

[Dan Leinir Turthra Jensen]

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


  Sorted, nicely done :) Makes the code just a touch simpler as well, which is 
always good :)

REPOSITORY
  R304 KNewStuff

BRANCH
  fix_isremote_stuff (branched from master)

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

To: alex, #knewstuff, ngraham, leinir
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29123: Do not mark entry as uninstalled if uninstallation script failed

[Dan Leinir Turthra Jensen]

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

INLINE COMMENTS

> installation.h:113
>   *
>   * The entry instance will be updated with any new information:
>   * 

Not blocking, but the documentation probably wants fixing before you push - 
just swap this line for "The entry emitted by signalEntryChanged (only happens 
when the uninstallation is completed with non-critical errors) will be updated 
with any new information, in particular the following:"

REPOSITORY
  R304 KNewStuff

BRANCH
  arcpatch-D29123_1

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

To: alex, #knewstuff, meven, ngraham, leinir
Cc: leinir, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29503: Pixel align children of GridViewInternal

[Kai Uwe Broulik]

broulik accepted this revision.

REPOSITORY
  R296 KDeclarative

BRANCH
  master

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

To: fvogt, #frameworks, broulik, mart, davidedmundson
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29503: Pixel align children of GridViewInternal

[David Edmundson]

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

REPOSITORY
  R296 KDeclarative

BRANCH
  master

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

To: fvogt, #frameworks, broulik, mart, davidedmundson
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29503: Pixel align children of GridViewInternal

[Fabian Vogt]

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

REVISION SUMMARY
  The scroll bar size can be odd (for breeze it's 21), which causes leftMargin
  to be 12.5. This causes every delegate inside to be blurred.

TEST PLAN
  Monkeypatched, now kcm_style is no longer blurred.

REPOSITORY
  R296 KDeclarative

BRANCH
  master

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

AFFECTED FILES
  src/qmlcontrols/kcmcontrols/qml/private/GridViewInternal.qml

To: fvogt, #frameworks, broulik, mart
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29451: KNS: Do not mark entry as installed if install script failed

[Dan Leinir Turthra Jensen]

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


  Thank you :D Land away :)

REPOSITORY
  R304 KNewStuff

BRANCH
  arcpatch-D29451

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

To: alex, #knewstuff, ngraham, leinir
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29451: KNS: Do not mark entry as installed if install script failed

[Alexander Lohnau]

alex updated this revision to Diff 82180.
alex added a comment.


  Use qOverload
  
  No problem :-)

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29451?vs=82177&id=82180

BRANCH
  arcpatch-D29451

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

AFFECTED FILES
  src/core/installation.cpp

To: alex, #knewstuff, ngraham, leinir
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

[Gustavo Carneiro]

araujoluis updated this revision to Diff 82179.
araujoluis added a comment.


  - restore code deleted by mistake

REPOSITORY
  R236 KWidgetsAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29502?vs=82178&id=82179

BRANCH
  named_color_support

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

AFFECTED FILES
  src/kcolorcombo.cpp
  src/kcolorcombo.h

To: araujoluis, tcanabrava, patrickelectric, hindenburg, ngraham
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

[Gustavo Carneiro]

araujoluis updated this revision to Diff 82178.
araujoluis added a comment.


  - Restore code deleted by mistake

REPOSITORY
  R236 KWidgetsAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29502?vs=82173&id=82178

BRANCH
  named_color_support

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

AFFECTED FILES
  src/kcolorcombo.cpp
  src/kcolorcombo.h

To: araujoluis, tcanabrava, patrickelectric, hindenburg, ngraham
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D25339: update lineHeight if boundingRect indicates a larger value.

[René J . V . Bertin]

rjvbb added a comment.


  With "we've ever seen" you do mean that lineheight only changes when a line 
that requires it scrolls into view?
  
  >   Though line height won't shrink during the edit phase, it will back to 
the actual value upon save.
  
  Have you tried to reset the max. lineheight on each redraw (I presume the 
scrollbars could give you a signal that a view scroll/jump was initiated, if 
need be). 
  Something tells me that it'd be nicer if lineheight always is as small as 
possible. Imagine you're using a smallish window to edit a document that just 
has some of these "offending", much higher characters at the bottom. If it gets 
into view only once, lineheight increases and it's as if you lose a lot of 
screen estate until you save the document. Now I mustn't be the only one who 
often doesn't save for a while, esp. when doing things like moving blocks of 
text around, and it's exactly in that kind of scenario where having to save to 
get a more space-efficient rendering back can be very annoying.
  
  As long as you can determine the max. lineheight required for the view that's 
about to be drawn before the view is actually drawn there should be no 
glitches. You'd just see a jump in lineheight and there would probably be an 
interesting problem to tackle with edge cases where the higher glyphs fall 
outside the view area because of the increased lineheight, but that's something 
your current implementation cannot avoid completely either. As to the changing 
lineheight: I think users will understand why it happens and get used to it. 
It's comparable to what you see in graphics programmes that show cursor 
co-ordinates next to the cursor; that indicator has to jump when it would get 
"pushed out" of the view, and that doesn't even feel weird.
  
  I presume your new approach would work not just for CJK characters, but also 
for anything else that changes the lineheight. That's and advantage but could 
also lead to regressions for some (who never use CJK characters or who, like 
me, wouldn't care how they display because they can't read them anyway). 
Emoticons come to mind; here too I don't really care if they get cut off. Scrap 
that, I *really* don't care if they are...

REPOSITORY
  R39 KTextEditor

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

To: xuetianweng, #ktexteditor, cullmann, dhaumann, #frameworks, rjvbb
Cc: brauch, sars, pshinjo, rjvbb, fakefred, anthonyfieroni, 
kde-frameworks-devel, kwrite-devel, rrosch, LeGast00n, cblack, domson, 
michaelh, ngraham, bruns, demsking, cullmann, dhaumann

D29451: KNS: Do not mark entry as installed if install script failed

[Dan Leinir Turthra Jensen]

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


  (oops, mistakenly marked as accepted, sorry...)

REPOSITORY
  R304 KNewStuff

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

To: alex, #knewstuff, ngraham, leinir
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29451: KNS: Do not mark entry as installed if install script failed

[Dan Leinir Turthra Jensen]

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


  In D29451#665420 , @alex wrote:
  
  > Fix connect
  >
  > Thanks, I wasn't aware of that until now :-)
  
  
  Same, until a couple of weeks ago :D i'd sort of... taken to doing it anyway, 
because it just seemed nice, but yup, turns out that you really definitely want 
to do that unless you know very precisely what you're doing :)

INLINE COMMENTS

> installation.cpp:355
>  QProcess* p = runPostInstallationCommand(installedFiles.size() 
> == 1 ? installedFiles.first() : targetPath);
> -connect(p, static_cast QProcess::ExitStatus)>(&QProcess::finished), this, installationFinished);
> +connect(p, static_cast QProcess::ExitStatus)>(&QProcess::finished), this,
> +[entry, installationFinished, this] (int exitCode, 
> QProcess::ExitStatus) {

Terribly sorry to keep doing this, but... yeah, noticed the old-style overload 
thing, but since we require higher than Qt 5.6 and already require a 
sufficiently high version of c++, we can use qOverload instead of the 
static_cast :) https://doc.qt.io/qt-5/qtglobal.html#qOverload

REPOSITORY
  R304 KNewStuff

BRANCH
  arcpatch-D29451

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

To: alex, #knewstuff, ngraham, leinir
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29451: KNS: Do not mark entry as installed if install script failed

[Alexander Lohnau]

alex updated this revision to Diff 82177.
alex added a comment.


  Fix connect
  
  Thanks, I wasn't aware of that until now :-)

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29451?vs=82100&id=82177

BRANCH
  arcpatch-D29451

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

AFFECTED FILES
  src/core/installation.cpp

To: alex, #knewstuff, ngraham, leinir
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29451: KNS: Do not mark entry as installed if install script failed

[Dan Leinir Turthra Jensen]

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

INLINE COMMENTS

> installation.cpp:355
>  QProcess* p = runPostInstallationCommand(installedFiles.size() 
> == 1 ? installedFiles.first() : targetPath);
> -connect(p, static_cast QProcess::ExitStatus)>(&QProcess::finished), this, installationFinished);
> +connect(p, static_cast QProcess::ExitStatus)>(&QProcess::finished),
> +[entry, installationFinished, this] (int exitCode, 
> QProcess::ExitStatus) {

Remember to give connect an object context for the slot (i did not realise 
until recently what leaving that out means, but it turns out to be potentially 
quite bad and crashy in difficult to track ways - in short, if our this 
instance is destroyed (say, the user quits the application) while an 
installation is ongoing, this would crash when attempting to emit or call 
installationFinished - it /should be fine, as i said, in most cases, as 
installation's a long lifetime object, but also just good style to add that 
context - see 
https://wiki.qt.io/New_Signal_Slot_Syntax#New:_connecting_to_simple_function 
for a detailed explanation, but in short, just add `this,` before the lambda 
and you're good to go :) )

REPOSITORY
  R304 KNewStuff

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

To: alex, #knewstuff, ngraham, leinir
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

[Gustavo Carneiro]

araujoluis edited the summary of this revision.

REPOSITORY
  R236 KWidgetsAddons

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

To: araujoluis, tcanabrava, patrickelectric, hindenburg, ngraham
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

[Gustavo Carneiro]

araujoluis edited the summary of this revision.

REPOSITORY
  R236 KWidgetsAddons

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

To: araujoluis, tcanabrava, patrickelectric, hindenburg, ngraham
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

[Gustavo Carneiro]

araujoluis edited the summary of this revision.

REPOSITORY
  R236 KWidgetsAddons

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

To: araujoluis, tcanabrava, patrickelectric, hindenburg, ngraham
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

[Gustavo Carneiro]

araujoluis edited the summary of this revision.

REPOSITORY
  R236 KWidgetsAddons

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

To: araujoluis, tcanabrava, patrickelectric, hindenburg, ngraham
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

[Gustavo Carneiro]

araujoluis created this revision.
araujoluis added reviewers: tcanabrava, patrickelectric, hindenburg, ngraham.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
araujoluis requested review of this revision.

REVISION SUMMARY
  Signed-off-by: Gustavo Carneiro 

REPOSITORY
  R236 KWidgetsAddons

BRANCH
  named_color_support

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

AFFECTED FILES
  src/kcolorcombo.cpp
  src/kcolorcombo.h

To: araujoluis, tcanabrava, patrickelectric, hindenburg, ngraham
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns