- User Since
- Apr 15 2015, 5:07 PM (136 w, 1 d)
- Implement latest VDG mockup.
For conceptual sanity checking, could you look at c9a7741f2b8082f025b5eb2e53c3c489a95e1da8 and how these align? Note how that commit cares more about what the user considers the visual center (the icon) rather than cell or delegate centers.
Thanks, will implement tomorrow!
Small comment change request in the comments, but code-wise good to go.
Modulo above edge-casey comments it looks good to me.
Tue, Nov 21
The above exchange is seriously upsetting and frustrating to me. I implemented the VDG mockup, and now there's discussion about the design in my code review. Please get your act together and make sure the mockups are ready for implementing so the review process of code work isn't this painful.
Address all review comments.
Mon, Nov 20
Review comments: Some of those also apply to the fonts KCM rewrite I based this on, e.g. the broken bindings. Will fix tomorrow, thanks :)
This is on git as branch kcm-redesign/launchfeedback.
Add missing package.
Fri, Nov 17
If the default sorting isn't on the checkbox, then I'm OK with and accept the original ver (not-so-fond of the variant).
Such a class would imho be pretty redundant to the model class, so now it's back to whether David wants to accept this patch or not.
I like it, but should the default sort column be the name so that there's no awkward arrow in the header element for the checkboxes?
Wed, Nov 15
Tue, Nov 14
I think we have the same problem in Kicker and Application Dashboard, which also use append trickery.
So if I understand correctly, the Mnemonic stuff currently doesn't seem to have the same aims as KAcceleratorManager does ...
Mon, Nov 13
I'm very happy to see this. I opened a thread about this on k-c-d (I think) last year, but I didn't follow through with actual code. We have various search fields in KDE that do something like "only search on length > 3", and they fail miserably with CJK input. KRunner is one such example.
Looking at the KActivities API more, perhaps not exposing this directly was deliberate of Ivan as otherwise Consumer would probably have all of the Cache signals. Ivan, can you opine on this?
I inquired upstream about a signal/API a few times and wasn't told about this signal. I will look into it, unless Ivan wants to.
I've found this problem in many KDE UIs, and I'd like to have a generic solution. How do you feel about adding something like a logicalStringLength() to KCoreAddons or KCodecs? Then we can use it in KRunner, KMail, Amarok, ...
Sun, Nov 12
Do you need someone to commit this for you or do you have access?
Sat, Nov 11
Fri, Nov 10
Thu, Nov 9
This makes sense to me conceptually. I can't comment on the flatpak recipe code though.
Wed, Nov 8
Please re-review, my above comment and diff update were submitted together.
Don't connect too rowsInserted/rowsRemoved.
Fix review comments by David and Kai.
Mon, Nov 6
And you just lost any chance of me accepting your patch, cf. https://www.kde.org/code-of-conduct/
Did you read the Phabricator ticket referenced in the original commit?
The original commit fixed broken RTL support. Did you test that this works with RTL? Because from a quick look it looks like you just reverted the code to its old, broken state.
Sun, Nov 5
This looks quite good to me now - the fact that it's much shorter than my old attempt feels pretty nice ;)
Fri, Nov 3
Thu, Nov 2
IRC talk for posterity:
Please put into wip/qtquick :)
What would you propose?
It's not necessary to delete connections. When a QObject is destroyed this is done already. Patch rejected sorry :)
Wed, Nov 1
I still don't like it, sorry - the patch also feels technically and semantically wrong to me. "If widgets are locked, stop adding a certain kind of data to the drag" is a pretty bad hack.
As-is I would not accept this patch, I'm sorry. Task Manager items are not widgets, it's content, and the widget mutability state should not affect content drags.
About those unreachable statements: Could you leave them in but comment them out? I have a feeling that was someone's poor attempt at leaving a TODO in ...
Lovely, again thanks so much for tackling this, it's long overdue.
Mon, Oct 30
One more newline after the emit line please, otherwise GTG.
Also handle empty roles.
Fri, Oct 27
This is nice, but it would be great to fix the other 20% of warnings it causes, too, otherwise it's a noisy build that hides more important warnings :)
Please don't extend already-accepted review requests with new code without requesting more review. Otherwise the status stays "Accepted" on code that hasn't actually been reviewed, which is confusing and holds things up :)