Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ToDo list #935

Closed
29 of 36 tasks
jarun opened this issue Apr 5, 2021 · 36 comments · Fixed by #1006
Closed
29 of 36 tasks

ToDo list #935

jarun opened this issue Apr 5, 2021 · 36 comments · Fixed by #1006
Labels

Comments

@jarun
Copy link
Owner

jarun commented Apr 5, 2021

Rolled from #881.

Cooking

Up for grabs

For anything else please discuss in this thread.

Contribution guideline.

@jarun jarun added the planning label Apr 5, 2021
@jarun jarun mentioned this issue Apr 5, 2021
20 tasks
@luukvbaal
Copy link
Collaborator

Following discussion from #267 I do think showing the the last characters of the file name should be prioritized in the case of file names longer than the name column. I was wondering if we should consider changing the unescape() function to return something like abcmnopqrst.txt or abc...pqrst.txt for abcdefghijklmnopqrst.txt.

@jarun
Copy link
Owner Author

jarun commented May 8, 2021

Doesn't cover all cases inclusively. For some filenames the difference may be in the middle. There's always f/^F for file details. Just leave it be.

@lawnowner
Copy link
Contributor

@jarun (#499 (comment)) The prompt looks better than before with repl now. This morning, I got a three line error message starting with "Assembler: " after running sleep 1; date and typing stuff while asleep, although all I can do is wishing you good luck with reproducing that as I can't even remember the full error message, let alone the keys pressed. As somebody who has been using nnn in monochrome, and with almost none of the official plugins, I guess the next release will be my favourite, except for the tilde contraction next to the context numbers; it looks almost like nothing whem I'm home.

@jarun
Copy link
Owner Author

jarun commented May 13, 2021

Get used to the tilde. It was requested by multiple users.

@Kabouik
Copy link
Collaborator

Kabouik commented May 13, 2021

I noticed preview-tui and preview-tui-ext queue image previews when scrolling through multiple files. It is probably a non-issue on most machines, but breaks previews on slower ones (i.e., SBCs running on SD cards, machines/VM/LXC without hardware acceleration for kitty image preview, etc.) because load times add up for every file hovered along the way.

It may be true for other file types, but is most visible for big images since they take longer to load (on my phone running Debian and previewing with kitty's icat, the issue is not huge with images around 500 KB, but very noticeable with images around 2-4 MB). I suppose even on real computers this could be an issue with large images from modern DSLRs or scanned files.

I tried:

        kitty +kitten icat --silent --place "$1"x"$2"@0x0 --transfer-mode=stream --stdin=no \
-           "$3"
        kitty +kitten icat --silent --place "$1"x"$2"@0x0 --transfer-mode=stream --stdin=no \
+           "$3" &

Which seemed to limit queued previews, but resulted in previews being shown only if scrolling slowly.

Should I open a dedicated issue about it?


While I mention preview plugins, how about making default bat previews more minimal with:

-           fifo_pager bat --terminal-width="$cols" --paging=never --decorations=always --color=always \
+           fifo_pager bat --terminal-width="$cols" --paging=never --style=numbers,header,plain --color=always \

It is a very minor difference and perhaps just personal taste, but I see no real advantage for the grid view by default:

Screenshot_20210513_006
Screenshot_20210513_005

@jarun
Copy link
Owner Author

jarun commented May 13, 2021

@leovilok @luukvbaal can you please take a look?

It s a very minor difference and perhaps just personal taste, but I see no real advantage for the grid view by default

Yes, it actually looks awful to anyone from sane more/less background. Please raise a PR to remove this from all plugins.

@luukvbaal
Copy link
Collaborator

luukvbaal commented May 13, 2021

The preview plugins work with a FIFO so the previews are inherently queued. Not sure anything can be done about a slow machine that can't keep up with how fast you change the selection.

Never noticed the bat defaults since I have BAT_STYLE exported in my profile. I agree the grid is only clutter so I'm fine with getting rid of it.

@Kabouik
Copy link
Collaborator

Kabouik commented May 13, 2021

I really have no clue how a FIFO works, but was hoping there was a way to interrupt prior previews if a new one is being issued, without waiting. Something I vaguely remember @leovilok solved with a simple & when I was having a similar issue with viu (hence why I tried & here, but I guess it's only a partial solution and reaches its limits when preview load time is longer).

[Edit] Similar issue with PDF and audio previews using poppler-utils andd ffmpeg in preview-tui-ext. Also I confirmed rapid image scrolling (using key repeat) causes it too on a fast computer, compared to the rather slow phone running Debian above.

@luukvbaal
Copy link
Collaborator

I was actually able to reproduce the queuing to some extent with kitty on my laptop with key-repeat and #1006 should fix the issue.

If at all possible I would suggest using ueberzug instead of kitty's icat if speed is an issue for you since it is ALOT faster.

@jarun
Copy link
Owner Author

jarun commented May 14, 2021

If at all possible I would suggest using ueberzug instead of kitty's icat if speed is an issue for you since it is ALOT faster.

Completely agree! As someone who multitasks all the time my experience with kitty has been horrible. And I have started compiling with O_FIFO=1 only after @luukvbaal added ueberzug support.

@KlzXS
Copy link
Collaborator

KlzXS commented May 14, 2021

TIL github will match "fix #XXX" in a middle of a sentence.

@KlzXS KlzXS reopened this May 14, 2021
@jarun
Copy link
Owner Author

jarun commented May 14, 2021

Nice image BTW @KlzXS!

@jarun
Copy link
Owner Author

jarun commented May 14, 2021

@luukvbaal and @Kabouik can you guys audit nuke + preview-tui* (all plugins in general) and see if those can be optimized?

Things I can think of:

  • case in place of too many ifs
  • use of shell builtins (e.g. type instead of which)
  • it would be great to get rid of echo for printf

This optimization document is a good start if you need any reference.

@luukvbaal
Copy link
Collaborator

Yeah some of those things have been bothering me since I started working on preview-tui but I didn't want to change the style you guys adopted. I'll see what can be done for preview-tui however I don't use nuke myself so I'd rather if someone else could pick that up.

@jarun
Copy link
Owner Author

jarun commented May 14, 2021

however I don't use nuke myself

Doesn't matter. Please have a look if you have the time.

@wustho
Copy link
Contributor

wustho commented May 14, 2021

@jarun hello there, sorry the thread is locked. About cleanfilename, with latest commit (merged) it won't overwrite any file...

Instead, it will be prepended with _ if file with the same name already exists... Eg. file1 --> _file1 if file1 already exists...

Already stated in doc:

#              And if there are two almost similar filenames
#              like: 'asd]f' and 'asd f' which both will be renamed to 'asd_f',
#              to avoid overwriting, the last file will be prepended by _.
#              So they will be: 'asd_f' and '_asd_f'
#

@jarun
Copy link
Owner Author

jarun commented May 14, 2021

@wustho awesome! Thanks for the confirmation.

@Kabouik
Copy link
Collaborator

Kabouik commented May 14, 2021

If at all possible I would suggest using ueberzug instead of kitty's icat if speed is an issue for you since it is ALOT faster.

Completely agree! As someone who multitasks all the time my experience with kitty has been horrible. And I have started compiling with O_FIFO=1 only after @luukvbaal added ueberzug support.

Ha, thanks for the advice! That's frustrating after I spent time configuring kitty over the last two days exactly because I wanted to use icat and finally have real image previews in terminal. I assumed icat being native to kitty, it would perform better than alternatives. I'll try the combination kitty + ueberzug on the slow machine instead.

@jarun what exactly was horrible for you with kitty? Overall performance regardless of icat?

@jarun
Copy link
Owner Author

jarun commented May 14, 2021

what exactly was horrible for you with kitty?

Image rendering performance.

@Kabouik
Copy link
Collaborator

Kabouik commented May 14, 2021

I failed at prioritizing ueberzug over kitty's icat in the plugin (I shifted its clause to the top but broke previews, won't bother you with that here), but to get a better idea of ueberzug's performance, I compared image preview between a fff fork using ueberzug and fff upstream using w3mimgdisplay.

I got much better rendering speed with w3mimgdisplay. Through my reading I learnt its advantages over ueberzug are it works on Wayland too, can print images on the tty, and doesn't need a daemon like ueberzug (apparently).

I am sure it has drawbacks too, ueberzug is vey popular. But would it make sense to add support for w3mimgdisplay in preview plugins? nnn has a broad audience that certainly includes slow systems (like SBCs), machines using Wayland, or others with just a tty, where both ueberzug and kitty would be unusable. Since w3mimgdisplay appears less restrictive and faster (at least I can distinguish it from ueberzug on a slow system, maybe not on a fast one), I guess it's not too redundant. viu could work on all those machines but there's a gap in functionality.

@Kabouik
Copy link
Collaborator

Kabouik commented May 14, 2021

w3mimgdisplay doesn't support tmux so problem solved!

@luukvbaal
Copy link
Collaborator

luukvbaal commented May 14, 2021

You're right, prioritzing ueberzug over icat in kitty requires more changes than shifting the order in image_preview(). See e.g. lines such as: [ "$TERMINAL" != "kitty" ] && exists ueberzug && ueberzug_remove in preview_fifo().

ueberzug previews are pretty much instantaneous on the machines I use nnn on but I can believe w3mimgdisplay could be faster as its written in C as opposed to python.

The problem I have with w3mimgdisplay is that I've never gotten it to work in any of the terminals I've used. This is also one of the main reasons ueberzug exists in the first place if you go by the README(it's definitely not less restirictive than w3mimgdisplay atleast for X11 environments).

Having said that, if you can manage to add support for w3mimgdisplay to preview-tui I say go ahead. Again, I've had nothing with problems with w3mimgdisplay and haven't touched it since ueberzug was created so I see no reason add it myself.

w3mimgdisplay doesn't support tmux so problem solved!

Yeah, considering that, w3mimgdisplay doesn't really fit in preview-tui I guess.

@luukvbaal
Copy link
Collaborator

@Kabouik This is the diff you would need for ueberzug previews in kitty.

@Kabouik
Copy link
Collaborator

Kabouik commented May 14, 2021

Nice, thanks a lot @luukvbaal! It's not instantaneous on my slow device but I can feel an improvement indeed.

The queuing is there though (which is expected, it was prevented just for icat as far as I understand). However I have mixed feelings about the queuing in this case. On the one hand it's ugly and can go wrong if scrolling for several seconds continuously [Edit: really wrong on long scrolls]. On the other hand ueberzug appears to create some kind of cache for the images it already previewed, which means this cache is fed by the queued files, so later preview of the scrolled files is almost instantaneous even on this machine.

@luukvbaal
Copy link
Collaborator

I do believe ueberzug does some kind of caching yeah but I would also suggest following
# Consider setting NNN_PREVIEWDIR to $XDG_CACHE_HOME/nnn/previews if you want to keep previews on disk between reboots on a slow device if you havent already. If disk space is less of an issue that is.

This will place converted images in $XDG_CACHE_HOME instead of the default $TMPDIR which is lost after a reboot.

@Kabouik
Copy link
Collaborator

Kabouik commented May 14, 2021

Will do. Disk space is an issue since this is a phone but I know where to clear the cache when I reach the limit.

Probably out of scope since it's not the official plugin, but it doesn't hurt to report it: with the diff to prioritize ueberzug over icat in kitty, the preview does not properly follow layout changes. See here with patched preview-tui-ext (ueberzug previews) on the left, and vanilla preview-tui (icat previews) on the right:

Peek.2021-05-15.00-23.mp4

(Similar issue when using tmux panes: the preview follows layout changes but is cropped in certain positions.)

@luukvbaal
Copy link
Collaborator

luukvbaal commented May 14, 2021

Yeah preview-tui is not aware of the preview pane origin as is the case for tmux. I already accounted for this for horizontal splits in the initial diff but forgot about vertical splits.

Try the following ueberzug_layer()

ueberzug_layer() {
    if [ "$TERMINAL" = "kitty" ]; then
        if [ "$SPLIT" = "v" ]; then
            x=$(($(tput cols) / 2 + 1))
            y=0
        else
            x=0
            y=$(($(tput lines) / 2 + 1))
        fi
    else
        x=0
        y=0
    fi
    printf '{"action": "add", "identifier": "nnn_ueberzug", "x": %d, "y": %d, "width": "%d", "height": "%d", "scaler": "fit_contain", "path": "%s"}\n' "$x" "$y" "$1" "$2" "$3" > "$FIFO_UEBERZUG"
}

Updated the gist as well.

@Kabouik
Copy link
Collaborator

Kabouik commented May 16, 2021

This didn't solve the issue from my first tests, but that was just a quick test while on the move, I'll test more thoroughly with more terminals and scenarios.

Unrelated: It was mentioned in #977 that scripts and vanilla functions outputs are garbled after several actions because they are displayed on top of each other. In cmusq I just added \n at the end of all prompts and this turned out to work decently for the use case. Would that be usable for nnn itself and possibly other scripts?

Example of garbled output with cpg here:
Screenshot_20210516_002
(By the way, noob question, how do we copy/paste from nnn and accept overwrite for the whole selection?)

I tried adding \n after overwrite? at L584 of nnn.c but it didn't work, I suppose the issue might be with cpg instead.

It may seem only an aesthetic issue but sometimes, depending on the length of sentences, it can be unreadable and it may be impossible to tell if the output expects an answer, what are the accepted answers, unless experienced with daily nnn use already.

@jarun
Copy link
Owner Author

jarun commented May 17, 2021

Would that be usable for nnn itself and possibly other scripts?

You can do that for all the scripts. Just ensure you see the problem in the script and then you do the change.

how do we copy/paste from nnn and accept overwrite for the whole selection?

This is not an nnn issue. nnn uses the interactive option of cp. See cp options. If you have something you'll have to recompile the code with that.

I tried adding \n after overwrite? at L584 of nnn.c but it didn't work

That prompt is from advcpmv. You can try to add an fflush() after every progress bar ending and see if that works. Please raise a PR if it works for you.

@mizlan
Copy link
Collaborator

mizlan commented May 18, 2021

May I request some better documentation in the form of docstrings and comments for variables and functions for the C source code? Since it isn't too clear what variable names mean at a glance, it's quite difficult to gain an understanding of the code from reading control flow alone.

@jarun
Copy link
Owner Author

jarun commented May 18, 2021

@Kabouik the broken overwrit eprompt is fixed at jarun/advcpmv@6adf544.

@mizlan unfortunately no such documentation exists. Please feel free to ask any questions you have. Also, I invited to the nnn devs group. Please join that. We generally discuss there.

@mizlan
Copy link
Collaborator

mizlan commented May 18, 2021

So sorry, but I am unaware of the dev group and can't seem to find it. What platform is it on? Gitter?

@Kabouik
Copy link
Collaborator

Kabouik commented May 18, 2021 via email

@jarun
Copy link
Owner Author

jarun commented May 18, 2021

but I am unaware of the dev group and can't seem to find it.

Please check your mail. I sent you an invitation 1/2 days back.

@mizlan
Copy link
Collaborator

mizlan commented May 19, 2021

The invitation for was collaboration (push access) right? I do not see any sort of discussion group associated with it. Sorry for the inconvenience.

@jarun
Copy link
Owner Author

jarun commented May 19, 2021

Not that one. I've resent the invitation just now. Please check.

@jarun jarun mentioned this issue May 19, 2021
50 tasks
@jarun jarun closed this as completed May 19, 2021
Repository owner locked and limited conversation to collaborators May 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants