Bug 407103

Summary: "Quick Open" useless in kate 18.12.3
Product: [Applications] kate Reporter: nfxjfg
Component: applicationAssignee: KWrite Developers <kwrite-bugs-null>
Status: RESOLVED FIXED    
Severity: major CC: christoph, gregap, kare.sars, kde, nate
Priority: NOR    
Version: 18.12.3   
Target Milestone: ---   
Platform: Ubuntu   
OS: Linux   
Latest Commit: Version Fixed In: 19.08
Sentry Crash Report:
Attachments: patch
patch2

Description nfxjfg 2019-04-30 15:48:45 UTC
SUMMARY

Kate 18.12.3 (and probably some earlier releases) break the Quick Open functionality fatally. It still "works", but not like it should be. I can observe two dealbreakers:

1. The file list is not LRU sorted. Instead it lists opened files first, and then unopened files, each sorted alphabetically.

2. No entry is selected by default. The second entry should be normally be selected by default. If you bring up "Quick Open" now and hit enter, it creates a new file. Every time. wat.

STEPS TO REPRODUCE

1. Start kate/a new session
2. Open a file (preferably a file from a git repository, with project plugin enabled)
3. Open a second file from the same directory
4. Open a third file from the same directory
5. Hit Quick Open, press enter
6. Hit Quick Open, press cursor key down once, press enter

OBSERVED RESULT

Step 5 creates a new file. Step 6 does nothing.

EXPECTED RESULT

Step 5. should open the second file (from step 3). Step 6 should open the first file (from step 2).

SOFTWARE/OS VERSIONS
Ubuntu 19.04 "disco": https://packages.ubuntu.com/disco/editors/kate

ADDITIONAL INFORMATION

I suspect this commit caused this: https://cgit.kde.org/kate.git/commit/kate?id=d6e38c0cbd3d6d7f7658862757b8c33db4322d7a

But I didn't try to bisect (is that even possible). I didn't try to build master on this non-Ubuntu either, I expect it'd be a nightmare anyway due to KDE frameworks versions.

The bug doesn't exist in 18.08.0 or before, didn't check other releases.

Should it turn out that nobody wants to fix it properly, I suggest reverting whatever caused it. If it was the commit I mentioned: The duplicating issue doesn't seem so important, since this bug exists in other places at least in Kate 18.12.3 anyway, and the speed win also isn't that decisive (still lagged when I tried it on a Linux kernel git checkout).
Comment 1 Nate Graham 2019-04-30 19:14:17 UTC
IIRC this was already fixed in 19.04, right Kate folks?
Comment 2 nfxjfg 2019-05-11 13:50:33 UTC
Are there any instructions for building kate git on debian unstable? Whenever I try the ones on the kate website, it's a mess with dependencies or cmake complaining about something.
Comment 3 Nate Graham 2019-05-11 14:13:26 UTC
Use the kdesrc-build tool. See these instructions: https://community.kde.org/Get_Involved/development
Comment 4 Kåre Särs 2019-05-13 07:00:31 UTC
You can also run:

sudo apt build-dep kate

To install all the dependencies for building kate. This requires that you have the source repositories enabled.
Comment 5 Dominik Haumann 2019-05-13 07:47:28 UTC
Is this fixed already, see bug 403097?

https://bugs.kde.org/show_bug.cgi?id=403097
Comment 6 nfxjfg 2019-05-13 16:08:56 UTC
> sudo apt build-dep kate

Well, now I managed to build it. I guess I gave up too early, and Debian removed these packages at some point in the past.

The problem still exists in git master as of today (kate cd54ede58ec80f1d10b). The kate about dialog shows version 19.07.70 (higher than any installed version). All problems I described still exist.

Maybe I'm still testing the wrong thing? It doesn't search for folders, even though 403097 should have been fixed.
Comment 7 Christoph Feck 2019-05-13 19:49:56 UTC
You could insert some qDebug() or printf() to make sure you are actually running the compiled version. Or check with strace which files are loaded.
Comment 8 nfxjfg 2019-05-14 17:19:57 UTC
It seems my observations were correct. I checked out a known working version, and it did work.

I bisected it:
90f51f5830e32998d4 breaks selecting the correct entry on show up
d6e38c0cbd3d6d7f76 breaks it completely

So I guessed right, except there was an earlier regression that also broke it somewhat.
Comment 9 Michal Humpula 2019-05-26 07:59:43 UTC
All the issues should be fixed on the latest master at this moment.
Comment 10 nfxjfg 2019-05-26 20:47:42 UTC
Thank your for looking at this. I update to kate git 10c552846e73e5ccb342d91d84ef4cf302750fab (didn't change ktexteditor), but unfortunately I can't observe any change. All things I listed are still broken.
Comment 11 nfxjfg 2019-05-26 20:53:48 UTC
Oh, sorry, I tested the wrong thing. It's still broken though.

This fixes "Step 5 creates a new file." (see my first post), but the implementation is wrong. It should do what I described under the "EXPECTED RESULT" heading. All other items are still broken.
Comment 12 nfxjfg 2019-06-25 15:41:33 UTC
Is anyone working on this, or should I try to fix this myself?
Comment 13 Christoph Feck 2019-06-25 15:45:36 UTC
If you would like to work on it, please assign this ticket to yourself. If you need help, please check https://community.kde.org/Get_Involved/development
Comment 14 nfxjfg 2019-06-27 11:52:58 UTC
Are there any directions what to do when you get thus message:

  Could not find a configuration file for package "ECM" that is compatible
  with requested version "5.59.0".

  The following configuration files were considered but not accepted:

    /usr/share/ECM/cmake/ECMConfig.cmake, version: 5.54.0

There's this kde-build thing, but it starts by building Qt (!). I don't understand this ECM thing. It's updated by a bot?
Comment 15 Christoph Feck 2019-06-27 15:16:54 UTC
Please check where to ask for help in https://community.kde.org/Get_Involved/development

The error message is self-explanatory. It expects version 5.59.0, but you only have 5.54.0. The solution is to either compile an older branch, or also compile/install the newest dependencies.
Comment 16 nfxjfg 2019-06-28 00:15:13 UTC
Here's an attempted fix: https://github.com/wm4/kate/commit/3f70b55f77c49593b8f9a17187a717a452915f79
Comment 17 Nate Graham 2019-06-28 01:37:43 UTC
Thanks! Please submit your patch using phabricator.kde.org. Here's the documentation for how to do it: https://community.kde.org/Infrastructure/Phabricator
Comment 18 nfxjfg 2019-06-28 11:17:26 UTC
After spending at least 5 minutes after solving these awful obnoxious captchas (that involve google spying on me and/or doing free work for google), and having to retry several times because the kde site didn't accept it (despite captcha marked as solved), I finally got through, and it sent me a notification mail. But when clicking on the confirmation link, the "Username" field is a drop down list with only 1 entry, "Not selected" (and I can't select anything else or enter anything). Clicking "Register Account" then shows this error:

Please fix the following input errors:

    Username cannot be blank.

I give up. This is too complicated for me.
Comment 19 Nate Graham 2019-06-28 18:17:34 UTC
I'm sorry you've had a bad experience. Please send an email to sysadmin@kde.org and they'll fix up everything for you!
Comment 20 Christoph Cullmann 2019-06-28 20:40:21 UTC
I can take a look at the diff on github, too.

https://github.com/wm4/kate/commit/3f70b55f77c49593b8f9a17187a717a452915f79

On the first glance, the comments about the wrong use of the unstable sort make sense to me, but I must give the patch a try first.
Comment 21 Christoph Cullmann 2019-06-28 20:54:58 UTC
I tried it, both master and your patch.
Your patch solves the described issues.
Would you be OK with me merging this?
Comment 22 Christoph Cullmann 2019-06-28 20:55:28 UTC
=> To merge I would just require a name + email address.
Comment 23 Christoph Cullmann 2019-06-28 21:01:07 UTC
Michal, could you try this patch, too?
For me, without + using master, the symptoms occur.
With the patch, e.g. the sorting is again LRU (aka least recently used or "usage order")
Comment 24 Michal Humpula 2019-06-29 08:06:14 UTC
I've tested the patch, most of the stuff seems ok, but I'm getting pre-selected second item (opened tab) all the time when I open quick open no matter which tab is active. Which seems counter intuitive for me (my expectation would be to preselect current tab). Master always selects first one. I guess the point was to be able switch between two tabs quickly? Isn't there a better way to do it then quick open?
Comment 25 Christoph Cullmann 2019-06-29 09:02:31 UTC
I actually though the pre-selected second element was before by intention to be able to quick-switch between the current and previous used document.

I don't think that is harmful, given as soon as you search the matches will be used.
Comment 26 nfxjfg 2019-06-29 18:59:05 UTC
I'd like to investigate whether the performance of this could be improved. My change doesn't make it slower, but it's something that probably needs to be done, and it might require intrusive changes.

If you use the project plugin with a very large project, quick open can take unacceptably long. I tested with the linux kernel (~65k files, ~130ms, feels annoying) and chromium (~300k files, ~800ms, not really acceptable). I suspect it spends at least half of the time in QFileInfo or QUrl, but not sure yet.

(There was some time when kate essentially froze even on the linux kernel, which I think provoked the change that unfortunately resulted in this bug report?)

Michal Humpula, if you open "quick open", and hit enter, it's supposed to jump to the previous file. Since the list entries are sorted by time of last use, the first entry is always the currently opened file, and the second is the previous file. I think this behavior is very useful, since it saves you a random "down" keystroke. The "Last Used Views" switcher works in a similar way and selects the second entry by default.

Selecting the first entry is just a regression.

We could also just not display the current file, or put it at the end of the list. That would do the same job, but maybe more awkward.
Comment 27 Christoph Cullmann 2019-06-30 16:46:38 UTC
I would appreciate further tunings.
But perhaps we shall first apply the patch to fix the behavior regressions.
Would you be ok with that and provide some name + mail address to push it?
Comment 28 Dominik Haumann 2019-07-04 09:29:24 UTC
ping
Comment 29 nfxjfg 2019-07-04 15:01:59 UTC
Created attachment 121321 [details]
patch
Comment 30 nfxjfg 2019-07-04 15:02:47 UTC
Here's a patch, I think the code is the same, commit message was modified. Feel free to apply it. Still haven't managed to create a KDE account.
Comment 31 Christoph Cullmann 2019-07-04 16:50:43 UTC
Hi, thanks.
I can push that for you, but I would need some name to attribute the change for the licensing.
Comment 32 nfxjfg 2019-07-04 17:28:43 UTC
Created attachment 121322 [details]
patch2

If you need an author name, here's a modified name with my real name as author name.
Comment 33 Christoph Cullmann 2019-07-04 20:13:18 UTC
Thanks, will apply this!
Comment 34 Christoph Cullmann 2019-07-04 20:16:09 UTC
The commit hook is unhappy with the e-mail address, can I use the one of the bugzilla comments here?
Comment 35 nfxjfg 2019-07-04 21:29:13 UTC
I'd rather not, this address is just supposed to be a spam sink, and I don't want to use it for "proper" purposes.  I don't have any other address I'd use for this purpose either (that's mostly a problem of the address being publicly listed in git forever), so I've set an obviously invalid address. Sorry for being so much work.
Comment 36 Christoph Cullmann 2019-07-04 21:35:28 UTC
Hmm, then we somehow have a small issue here ;=)

I mean, I can commit this with me as author, given the ip rights of this change are limited anyways, but that would be a bit strange, too.
Comment 37 Christoph Cullmann 2019-07-10 17:58:44 UTC
I think I just push that with mentioning you in the commit message and we are done.
Thanks for the contribution.
Comment 38 Christoph Cullmann 2019-07-10 18:00:08 UTC
Git commit 2fdd8c14268e8af93e6515d916db6e72cccc7a7b by Christoph Cullmann.
Committed on 10/07/2019 at 17:59.
Pushed by cullmann into branch 'master'.

[PATCH] Quick Open: fix LRU listing regression

The quick open list used to be sorted by order of access (the old code
called it LRU order). Commit d6e38c0cbd3d6d7f76 broke this.

The first sort() call is changed to stable_sort() in order to preserve
the "bold" field and the new "sort_id" field. A comment warns about this
subtle and easy to break requirement. This change was needed to sort the
file list by LRU (files not in the sortedViews list have no sort_id set,
and thus are sorted below all LRU sorted entries).

stable_sort() should be slower than sort(), although the C++ standard
promises the same algorithmic complexity. On the other hand, this change
also lets us get rid of the openedUrls string list and the associated
linear search.

The second sort must always be a stable_sort(). This is a bug in the
previous code. It sorted only on the "bold" field, which means
everything else can be reordered as sort() likes. Even if it "worked",
it was buggy.

To completely restore the old behavior, select the second entry by
default in the quick open list. This is so that you can quickly switch
between the two last recently accessed files. The old code actually
selected the first entry if the sortedViews list contained less than 2
elements - keep that behavior too.

Author: Vincent Lang

M  +5    -1    kate/katequickopen.cpp
M  +13   -16   kate/katequickopenmodel.cpp
M  +1    -0    kate/katequickopenmodel.h

https://commits.kde.org/kate/2fdd8c14268e8af93e6515d916db6e72cccc7a7b
Comment 39 nfxjfg 2019-07-10 19:44:54 UTC
Thanks.
Comment 40 Christoph Cullmann 2019-07-13 19:16:54 UTC
*** Bug 404212 has been marked as a duplicate of this bug. ***