Bug 416376 - FileFilter only filters files in the current working directory, but not sub-directories
Summary: FileFilter only filters files in the current working directory, but not sub-d...
Status: REOPENED
Alias: None
Product: konsole
Classification: Applications
Component: general (show other bugs)
Version: unspecified
Platform: Other Linux
: NOR normal
Target Milestone: ---
Assignee: Konsole Developer
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-01-17 11:51 UTC by FeepingCreature
Modified: 2020-12-12 06:50 UTC (History)
2 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description FeepingCreature 2020-01-17 11:51:01 UTC
SUMMARY

Files in the current folder are underlined fine. However, files in subfolders are not.

STEPS TO REPRODUCE
1. Turn on "underline files" in the "mouse" page of the profile settings
2. mkdir foo
3. touch foo/bar.txt
4. echo foo/bar.txt

OBSERVED RESULT

foo/bar.txt is not underlined and cannot be click-opened.

EXPECTED RESULT

foo/bar.txt should be underlined.

NOTES

The FileFilter only recognizes files in the current directory. Instead of checking for that list of files, it should chop off "foo/", check that "foo" is a folder in the current directory, and if yes, check the whole name. This should have low performance impact.

BENEFITS

kdevelop's konsole kpart would get "click error to open file" for free.
Comment 1 Kurt Hindenburg 2020-03-22 17:32:34 UTC
Thanks
Comment 2 Ahmad Samir 2020-03-22 23:36:20 UTC
Git commit 26f4a2218f52f790d4de087216098619a753164e by Ahmad Samir.
Committed on 22/03/2020 at 18:05.
Pushed by hindenburg into branch 'master'.

[FileFilter] Create HotSpot's for files in child dirs too

When creating HotSpot's for local files, create them for files in the
current dir and for files in sub-directories too.

Re-format the code used to build the regex pattern for more readability
(easier for seeing what the regex pattern will look like, I hope).

Use a static QRegularExpression object, so as to only construct it once,
the pattern doesn't change and this should help with performance. Also
make createFileRegex() not static.

Don't convert QList<QString> to QSet<QString>, without actual benchmarking
it could be that QList is actually efficient here.

Use const where appropriate.

M  +47   -37   src/Filter.cpp
M  +1    -1    src/Filter.h

https://invent.kde.org/kde/konsole/commit/26f4a2218f52f790d4de087216098619a753164e
Comment 3 FeepingCreature 2020-03-23 07:09:14 UTC
In the case that a filename matched one of the things in the current directory, you still need to check that it actually exists, though, right?
Comment 4 Ahmad Samir 2020-03-23 07:49:05 UTC
With this patch:
- we get the list of files in the current dir, these are underlined (previous behaviour)
- we get the list of sub-dirs and underline all files that have a path that begins with one of the sub-dir names, but we don't check that any files in the sub-dirs actually exist.

It's not ideal, but it's a trade-off, listing all the sub-dirs would be costly performance-wise (and how much depth? 2-3 folders deep or more?), and the use cases of e.g. using 'ls', or files in the output of 'grep' would work.
Comment 5 FeepingCreature 2020-03-23 08:43:21 UTC
Why not just check if the file exists once you encounter it? There's no need to build a recursive list in advance.
Comment 6 Ahmad Samir 2020-03-23 08:48:29 UTC
(In reply to FeepingCreature from comment #5)
> Why not just check if the file exists once you encounter it?
Performance: https://cgit.kde.org/konsole.git/commit/?id=2322c5605ea267e766f8795731263738f9e60dcc
Comment 7 Ahmad Samir 2020-03-23 08:49:25 UTC
And patches to improve file path matchinh are welcome of course. :)
Comment 8 Vadim A. Misbakh-Soloviov (mva) 2020-12-12 06:01:39 UTC
by the way, one of the mentioned commits totally brake my experience with konsole ;)

I had a directory named `t` in my homedir ad some crap inside it.

Also, I'm using weechat, poezio, and sometimes mutt and lynx.

So, every time I got there (in any of them) a text that contains two or more "don't" words, all the text (even f it takes the half of visible screen) gots underlined on mouse over :D

And clicking it makes an error like "there is no file /home/mva/t bla bla bla bla bla bla foo bar moo meow ...".


So, I think those fixes needs a fix themselves ;)
Comment 9 Vadim A. Misbakh-Soloviov (mva) 2020-12-12 06:26:54 UTC
Well, I think, if it at would least check for "/" after "t", that would be more correct behaviour and I would not encounter that bug.


// by the way, it would also be nice to react on relative ("./file.moo") and global ("/home/user/file.moo") paths ;)
Comment 10 Vadim A. Misbakh-Soloviov (mva) 2020-12-12 06:50:29 UTC
btw, I just found that filter matches (and so konsole underlines) **ALL** the strings **starting with** known file name (not only between ''s).

Preconditions:
I have "dev" and "1" directories in home dir. Also there is some files named "2", "4", "5" (notice, no "3").

Situation:
I opened tmux, changed dir to one of my git repos (here konsole still thinks I'm in home dir, since tmux was opened there). Let it be the Gentoo portage tree (package build scripts, sorted by dirs). Then I pulled changes, and looking git log.

Here, all the path like "dev-python", "dev-lua", "dev-libs" and whatsoever - underlines (and files to open since konsole tries to open it in relation to home dir).

Also things like "100%", "247483" ("... files"), and so on (starting with 1,2,4,5) - underlines too. And yes, "3249" - is not underlined.


So, I think, it definitelly should not work that way (match all that starts with substring that equals to known file).
At least, I think, for dirs it definitelly needs to check for "/" after the name.

As for files, I think it should stop the match on last character of the filename (so, if suspected string is longer, it will not match).

Although, there is a neighbour bug around, that requests filter to support line/columns specifications (like in compiler output: `my/source/file.c:21:37`) to and being able to pass that line/col numbers to the custom command.
I think, that should also be taken in mind when fixing