- User Since
- Jul 30 2015, 8:46 PM (150 w, 3 d)
Looks good for me!
Thanks for the improvements.
Sat, Jun 16
Will take a look ;=)
Thu, Jun 14
Nothing to be sorry about ;=)
Thanks for taking care!
Hmm, we want to have it in master, too, or?
Wed, Jun 13
If that change works in old Qt and new, I am happy with it.
Sat, Jun 9
I wonder if that really makes sense, given not all blocks contain ranges at all and they not really correlate with the block size.
Does that change really make it much faster or just shift the costs?
I think the idea was that it is an easy way out of the completion.
I am not sure if wrapping is what one would expect.
Thu, Jun 7
I followed the "I think it was agreed this is an improvement, so i'm going to suggest we commit it." comment from above.
In any case, it is an improvement to the old situation.
I can push that change, if OK.
Sun, May 27
> What should we do with this?
Thu, May 24
You still work on that or shall I approve?
Thanks for the improvements!
Looks good, beside one thing, but I might be wrong: Wasn't "<guimenuitem>Show Word Count</guimenuitem>" removed again from the menu and added as config item?
We might revert that and add it back, thought.
May 11 2018
Just missed that.
Why not ;)
May 4 2018
If the stuff still works for you and is fast now, I am still for merging that.
If somebody has time to do a even nicer integration, a new request can be done afterwards.
Beside that, I am not sure how you handle view deletes, isn't the current view pointer then dangling?
Hmm, of example in the
May 3 2018
Actually, given the functionality is only used there, if that code is in the view or in that helper class makes no real difference, or?
If you have benchmark results for that, I have no issues with the idea.
Thought I think you only need to update the cache in add/removeView, not in KTextEditor::DocumentPrivate::createView
Oh, good point by the way: Are you sure this should be an interface to a *view*? Maybe it should instead be attached to the document? I'm not sure, I just want to bring the discussion up.
Actually, I think view might make sense, as you only mess with the layouts and renderer.
But the implementation ATM makes it a interface of the document, or I am mistaken?
Hi, I think the reference to kateviewmanager.cpp is a good hint for how to implement it "correctly".
Unfortunately the proper xmlgui client handling is a bit hard.
You always need to add/remove the clients from your factory.
As you see, kate uses some class to disable updates during that process, to avoid nasty toolbar/menu flickering as comfort feature.
If you properly remove the stuff, no crashs shall happen. At least I am not aware of such issues.
May 2 2018
Hmm, I don't understand that comment?
Now you have one (or more) object per view, or?
That looks sane to me.
Or do I misunderstand how that works?
I am ok with it as it is ;=)
I would have been ok with the version that still inherits from QObject, too, but now that one has done all the hassle to avoid that ok ;)
Why not ;=)
Apr 26 2018
Any reason you guys decided to not involve email@example.com ?
I think we all forgot to do that, without any real reason, I drop that address a mail now, thanks for the hint!
I have no strong opinions on that, as I am not really using the line numbers in such a way.
But I think it can't hurt to have this in.
Apr 25 2018
I will ask the openSUSE engineer Matthias Gerstner for feedback before landing this.
Why dropping syncToDisk? Why is that related to the current issue?
Because it always just worked on a invalid file handle.
That is just wrong.
One can add it to an other place fixed later if one really needs it.
The only thing that is unclear:
Should solve all reported issues.
Hi, I propose this variant, that should remove all issues report.
Apr 16 2018
Given we have unit tests + the static checker is happy (I assume), feel free to commit.
Apr 15 2018
This is already pushed ;=)
Perhaps the fix is only a hack, but at least it solves the issue for me on my company workstation ;=)
I was confused what did look strange after the upgrade, until I noticed the missing bold keywords.
Git commit 4d91fa7e918d983e6569798dfe20c7c9faf4bb9e by Christoph Cullmann.
Committed on 15/04/2018 at 11:36.
Pushed by cullmann into branch 'master'.
Btw there are bug reports for this. Could you link the Kate related ones, if existing?
I failed to find them :(
I remembered that this was brought up some time ago, but I had no time to take a look at that time.
And now I no longer find the mails/bugs :P
Yes, please push your change.
Thanks for the work!
Apr 9 2018
You can use a mutable member for that.
Hi, must the variable be static or would an object member be good enough?
Apr 8 2018
And the auto-test does have diffs like:
Ah, I let just the build run once more locally, the current static check says:
Great!, This is my first contribution to The KDE project, I'm very happy you accepted it! :)
I assume that means I shall push?
instead of duplicating it further, could we move the code into ktexteditor and make it reusable from there?
Yep, +1 for that.
I think it looks ok, no hard-coded colors, it seems to pass the basic static checks and we have a testcase.
I would more like to have a test case in plain english, but the text just seems to be some random excert out of some old book, if google translate doesn't cheat me.
I appreciate the intend to speed this up.
But I think, to do this proper, it might make more sense to cache the current position in the KateViewAccessible and invalidate it on changes to the document.
The way you do it now, it might not be correct if text changes occur in-between. (if I don't overlook something)
Sorry, for the tests: inside the autotests, there is input/reference/html
Just add your testcase in input, run the tests, take a look at the results and check them in as references.
The hard-coded colors got removed, test is there, all fine.
Thanks for the contribution!
If you can't push youself, tell me, I will push for you.
Apr 5 2018
If that stuff is not around, why not, better than no compilation.
Thanks for taking care!
Apr 4 2018
Yeah, makes sense.
Thanks for the contribution.
Sure, perhaps Kevin as "Manifesto" expert can take a short look if he thinks all is in order?
I am not aware of any issues, but perhaps a second opinion would be good.
Apr 3 2018
I think allowing a own configuration per hosting application makes sense.
But I would propose to do this "opt-in".
We could add some application wide flag in the config that triggers "application local" storage, that is default off.
The KTextEditor config pages could allow to change it, if e.g. Kile wants to have this on per default, it just needs to adjust the default by shipping a default kilerc.
Mar 26 2018
Mar 23 2018
Yep, makes sense.
Thanks for the improvement.
Mar 22 2018
For the sorting/overlapping question: actually the HL code should ensure there are neither overlaps nor unsorted entries.
And I think if there are, already now "strange" things happen, as normally the first item would win.
Mar 21 2018
Ok, that is true, for the "degenerated" very long line case it makes sense to do the binary search.
But I don't think you should do that on your own, lower_bound is good enough, we use that for other things like the folding, too, if I am not mistaken.
I still am not that convinced that binary search helps for vectors which normally have only << 10-20 elements for normal long text lines.
But I would be ok with some range based for variant, as the loop with index increment is even ugly without any speed in mind ;=)
Mar 20 2018
Hmm, does it really get that much faster or is the stuff just moved to the subroutines?
I would not have thought that something is faster than simple linear scan for a short vector.
Mar 19 2018
Could one use a C++11 range-based for?
Given the container is const in that context, it won't detach, or?
Mar 18 2018
If we want that, one needs to take care of the config loading/initial setup of the m_presentationModel in a different way than relying on the config widget to do it.
Git commit 7f3d9e774129618dfb9fd871d5d5c8fbb66b4d9a by Christoph Cullmann.
Committed on 18/03/2018 at 12:33.
Pushed by cullmann into branch 'master'.
Perhaps one has not thought about the side-effects of the construction of the KateCompletionConfig.
It looks like it does a readConfig and that triggers applyInternal() which in return will configure the m_presentationModel.
I will revert this commit for the time being.
Mar 13 2018
I think it is ok that way, too ;=)
Mar 5 2018
I did some test today on tbaloo and noticed one problem when fetching the results from a query. The paths are encoded like URLs but without the scheme. I had to modify my code to use it. Apart from that, nice work. It just works. Are you still interested to work on that ?
I would be willing to work more on that, if there is consensus that it should replace baloo and we use the synergy with tracker instead of doing all things tracker does just once again for the sake of it (beside running in all the same security problems).
But until now, that doesn't look like there is such a consensus.
Feb 26 2018
Default is 1, in readConfig, given that should overwrite it, leave it 0 or change it to 1 if you like.
Feb 25 2018
Btw., if you want to see the joys of utf-8 on Windows, read: http://utf8everywhere.org/
Windows never uses UTF-8 ;=) Even NTFS uses UTF16/UCS2, but not UTF-8.
But git always uses utf-8 for its stuff.
Feb 13 2018
No idea ;=)
Then lets reopen it.
Feb 11 2018
I think they are not exactly the same. physicalDotsPerInchY() would give twice difference in the resolution depending on whether one uses the native or HiDPI mode. But logicalDpiX()/logicalDpiY() would return the same value in both cases.
No, I meant: if the logical variant works for us here, the physical variant should work fine, too.
That they not give you the same results in all cases is clear, given they are logical vs. physical.
I would go for the Qt functions, as their results are consistent with what Qt itself will later use anyways.
My only nitpick is why one goes over QScreen and not just asks the widget.
In our company we use the logicalDpiX()/logicalDpiY() on the widget we paint to, that works on macOS, too.
Feb 6 2018
I just tried in on Windows, its UTF-8.
If it is UTF-8 even there, we should just convert from that instead.
Feb 5 2018
I think beside the
+1, Volker, ok to go in?
Why not, if it helps some people's workflow.
If the new highlighting better covers what CMake has around on features now than the old, I am for adding the new one.
Hmm, is the encoding of the file names then the local 8 bit variant? Or is it utf-8 everywhere?
On Windows I am always bit confused when to use local8Bit.
Feb 2 2018
Jan 28 2018
Thanks for testing, will apply that then now ;=)
Jan 27 2018
Either this patch can improve that or make it worse ;=)
I tried out some .desktop file with different translations inside that have "complex" glyphs and I think it works better now.
If you have time to test that, it would be great.
Jan 23 2018
Less duplication => good!
Yeah, logic did seem to be flawed.
To only look at the line makes sense.