User Details
- User Since
- Apr 20 2015, 7:20 AM (465 w, 23 h)
- Availability
- Available
Nov 12 2020
May 16 2020
I like this patch. In fact, I observed over the past years again and again that sometimes, especially if chinese or similar letters are included, the baseline is wrong in Kate, leading to much more overpainting that needed.
If this patch fixes this, then I'm all for it. Or let's put it like this: The current implementation is wrong, this patch looks less wrong than the current state :-)
May 15 2020
I suggest to revert, and send a notification with the change to kde-distro-packager@kde.org to avoid that many users break their configuration.
May 14 2020
@cullmann could you integrate this?
Good catch :) please commit
May 9 2020
Mar 25 2020
I am currently not convinced for several reasons:
- There is already a way to do this on KTextEditor level, see: https://github.com/KDE/ktexteditor/blob/master/src/document/katedocument.h#L406
- I believe this should be a plugin, since the current implementation makes the current code more complex, and it's a feature we have not needed for > 15 years.
Mar 14 2020
Thanks yes, maybe you can add a comment to the skipOffset, Christoph :)
Mar 13 2020
I guess this is OK, but the concept of a "skip offset" is a bit fuzzy to me.
I guess in a followup commit the reported issues will be fixed? :-)
Mar 10 2020
The intimidating one :)
Mar 9 2020
Almost good, please fix + ship.
I like the patch, but please add a unit test before we commit this. See https://github.com/KDE/ktexteditor/tree/master/autotests/src/vimode
Could you add one? :)
I guess we can give this a try. As I understand, though, with this patch you will never be able to use e.g. a global zoom once you change the zoom of a view. This was different before this patch.
Feb 26 2020
I'd be ok with this for Kate.
Feb 23 2020
Feb 22 2020
Ok, then I am fine with this. Maybe add a KF6 task to the KF6 board?
Feb 21 2020
Definitely a +1 for using a QIcon here.
Feb 13 2020
Ok, I now fully got this, thanks for the explanation. And indeed without any left fill visual feedback is missing.
Feb 12 2020
No offense meant, but even with the screenshots I still have no idea what this is about :)
Feb 9 2020
I think this patch looks good.
Feb 6 2020
I'd be fine with committing this. Still, there very clearly is large room to improve this further.
I like the idea of the patch, could you add a screenshot?
Jan 30 2020
Better :) with corners I mean the 1-3 pixels left due to the rounding corners. These pixels were once also drawn as background although they are outside of the frame. It may be a minor detail, but imho such details are important. But indeed, the screenshots look good.
In two days we have the next KDE Frameworks tag. I'd have preferred a commit on Sunday ;)
Can you provide screenshots of more styles? :-)
Does that also work in Kate for floating messages like when the search wraps? What I want to know is whether the corners behind the green frame are transparent in this case, or whether the corners are painted solid. If I remember correctly, these kind of bugs were the reason to use Qt StyleSheets. And it must work with all styles.
Jan 24 2020
Looks good to me, and very much in line with the other canConvert statements before.
Jan 21 2020
Jan 20 2020
Looks good to me. Can you commit or shall we push this for you?
Jan 19 2020
Well, it definitely would be very nice if you find a patch that fixes this.
In that case I withdraw my review for now. I can only speak for Kate that I could not find a regression with my tests. But if you find other regressions, then this is likely not correct.
I just tested with KTextEditor's messagetest and KTextEditor's usage of KMessageWidget in the top and bottom bar, and all this looks still good.
Since KTextEditor uses KMessageWidget extensively we have to make sure the unit test messagetest still passes with this change.
Jan 16 2020
I guess this change is correct :-)
Jan 13 2020
That looks already better: If a line is wrapped and on the same page, then only the selected text is printed. It seems there are still corner cases.
- Create a document with just one long line that wraps over two printed pages. I this case, I am not able to print only the selected text properly.
- Say you have a line that wraps over e.g. 5 visual lines. And you just want to print the visually wrapped line 4 and 5. Right now, the visual lines 1-3 are just empty, but still take place. Maybe it makes sense to omit fully empty lines completely?
Jan 10 2020
Jan 6 2020
Jan 5 2020
Jan 3 2020
Jan 1 2020
Ok with this... Thanks. A note " Since 5.6x would be nice".
Dec 30 2019
Dec 29 2019
@hoffmannrobert: by the way, looking at your phabricator activity, you should get a KDE contributor account, if you don't have one already.
@hoffmannrobert: Are you maybe also interested in looking into https://bugs.kde.org/show_bug.cgi?id=415570 ? It's again about printing, this time about the very last line that seems to be wrong...
Dec 22 2019
@Gregor: That the terminal does not get focus on show was a regression in konsole part and should be fixed. I don't think there is any action item left for now.
Dec 15 2019
Dec 11 2019
Dec 10 2019
Was anything of the previously commented issues addressed?
Dec 8 2019
Doesn't this patch imply we have the same action twice now? Once provided by Konsole, and once by katemdi via View > Tool Views > Show Terminal?
Dec 6 2019
Dec 5 2019
Do we dislike iterators now?
We don't, and they still make sense for when you need the key, but range for is just much nier to look at :)
I'm fine with that statement. But are we going to be reviewing changing all the KDE code from iterators to range for? Feels like an overkill to me.
Looks good. But does the highlighting work for RW+CD? I am wondering whether + needs to be added to the weakDeliminator list?
Dec 4 2019
You can achieve Dolphin's behavior by reassigning the F4 shortcut to View > Tool Views > Show/Hide Konsole. That's what I do since > 10 years, and it works perfectly. Why is it not the default? Maybe because this action does not exist by default (i.e. when the Konsole plugin is not loaded). But the right fix is to reassign the F4 shortcut to this action.
Dec 3 2019
Ok
Dec 2 2019
Nov 26 2019
@jhayes Do you have commit access, or shall I commit this for you?
Any news here?
I think we should decide what to do with this patch, as over time it will get merge conflicts.
Nov 24 2019
I think this is ok.
Nov 23 2019
Makes sense, thanks.
Nov 22 2019
Lgtm.
Nov 19 2019
Nov 16 2019
Can't you call rehighlight() yourself after calling setDefinition()?
You can force the current clang format to keep the multi-line if as follows:
Nov 11 2019
Nov 10 2019
@hein: Could you add a commit that relicenses all code to MIT? We switched to MIT for entire KSyntaxHighlighting. This will ease integration later.