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

(BUG) The bottom status bar is not updated #1087

Closed
og900aero opened this issue Jan 20, 2023 · 15 comments · Fixed by #1149
Closed

(BUG) The bottom status bar is not updated #1087

og900aero opened this issue Jan 20, 2023 · 15 comments · Fixed by #1149

Comments

@og900aero
Copy link

The authorization list of files and directories does not change, even if I use the reload command. Here is the example command I created:

cmd change_permission ${{
	clear
	set -f
	printf "New permission -> "
	read perm
    for f in $fx; do
        sudo -A chmod "$perm" "$f"
    done
    lf -remote "send reload"
    lf -remote "send unselect"
    lf -remote "send clear"
}}

Even if I use the reload command, the lower status shows the old authorization, while the color of the file changes according to the authorization. If I move the cursor back and forth, the authorization display on the lower status bar is also updated.

@DusanLesan
Copy link

DusanLesan commented Jan 24, 2023

Maybe this works for you:

map b push :new_permission<space>

cmd new_permission &{{
	for file in $fx; do
		sudo -A chmod "$1" "$file"
	done
	lf -remote "send $id reload"
	lf -remote "send $id reload"
}}

I have no clue why two separate reload commands work.
This may also fix #1079

@og900aero
Copy link
Author

Working great, thank u!
Is it possible to use the text "New permission ->" instead of the name new_permission? Or does the name of the command and the parameterization of push not allow the use of space etc characters?

@DusanLesan
Copy link

Name of the function does not allow use of spaces but you could use non-breaking space instead. So using map S push :New permission -><space> and cmd New permission -> &{{ would look like this:
image

@og900aero
Copy link
Author

Ohh, working, thank you! :)

@DusanLesan
Copy link

Maybe this one needs to be reopened since calling same command twice in a row does not sound OK to me.

@og900aero og900aero reopened this Jan 26, 2023
@og900aero
Copy link
Author

Okay, reopened.

@DusanLesan
Copy link

These are even simpler steps:

  • Open new terminal instance and command: echo test >> ~/log
  • Open lf and focus new file
  • Wait a minute
  • Repeat echo test >> ~/log command from other terminal
  • Give lf reload command (either remote or directly)

Actual result: Date modified is the same. Only after second reload file information is updated in status bar.

Expected result: Only one reload should be required to fully reload status bar, preview and file colors.

@joelim-work
Copy link
Collaborator

Hi @DusanLesan , I believe I have found the issue. Below is an explanation of what is happening.

  1. Displaying the file information is handled by the loadFileInfo function, which selects the file at the current posiiton and displays its (cached) information:

    lf/ui.go

    Lines 727 to 750 in 3fc1db5

    func (ui *ui) loadFileInfo(nav *nav) {
    if !nav.init {
    return
    }
    curr, err := nav.currFile()
    if err != nil {
    return
    }
    var linkTarget string
    if curr.linkTarget != "" {
    linkTarget = " -> " + curr.linkTarget
    }
    ui.echof("%v %v%v%v%4s %v%s",
    curr.Mode(),
    linkCount(curr), // optional
    userName(curr), // optional
    groupName(curr), // optional
    humanize(curr.Size()),
    curr.ModTime().Format(gOpts.timefmt),
    linkTarget)
    }

  2. Although loadFileInfo is called after a reload, because the reloading happens asynchronously, the old file information will be displayed:

    lf/eval.go

    Lines 1307 to 1315 in 3fc1db5

    case "reload":
    if !app.nav.init {
    return
    }
    if err := app.nav.reload(); err != nil {
    app.ui.echoerrf("reload: %s", err)
    }
    app.ui.loadFile(app, true)
    app.ui.loadFileInfo(app.nav)

  3. During the actual asynchronous reload, the current file is determined again, but loadFileInfo is only called if the file information (app.ui.msg) is blank, which is not the case due to step 2:

    lf/app.go

    Lines 413 to 420 in 3fc1db5

    curr, err := app.nav.currFile()
    if err == nil {
    if d.path == app.nav.currDir().path {
    app.ui.loadFile(app, true)
    if app.ui.msg == "" {
    app.ui.loadFileInfo(app.nav)
    }
    }


I think in order to solve this, we would have to either:

  • Remove the if app.ui.msg == "" condition in step 3. But this could cause an undesirable side effect where app.ui.msg is updated unconditionally (potentially wiping out something like an error message) whenever an asynchronous reload happens.
  • Clear app.ui.msg instead of calling loadFileInfo in step 2, which means the if app.ui.msg == "" condition will end up being true. I think this approach is safer.

@DusanLesan
Copy link

@joelim-work Thank you for investigating this issue. I am sure it will help someone in fixing it.

@joelim-work
Copy link
Collaborator

@DusanLesan Would you by any chance be able to test the suggested changes to see if it fixes the issue? If it works for you, I wouldn't mind submitting a pull request for this (most likely the second approach).

@DusanLesan
Copy link

@joelim-work I would be ok with that if you give me a patch with changes. You can start with second one if you prefer it. Also, let me know if other features could be affected so I could try them too.

@joelim-work
Copy link
Collaborator

Hi @DusanLesan

Here you go, apologies in advance for the large wall of text that follows 😛


Solution 1:

Remove the if app.ui.msg == "" condition in step 3. But this could cause an undesirable side effect where app.ui.msg is updated unconditionally (potentially wiping out something like an error message) whenever an asynchronous reload happens.

Checkout this branch: https://github.com/joelim-work/lf/tree/reload-status-bar-1

--- a/app.go
+++ b/app.go
@@ -414,9 +414,7 @@ func (app *app) loop() {
                        if err == nil {
                                if d.path == app.nav.currDir().path {
                                        app.ui.loadFile(app, true)
-                                       if app.ui.msg == "" {
-                                               app.ui.loadFileInfo(app.nav)
-                                       }
+                                       app.ui.loadFileInfo(app.nav)
                                }
                                if d.path == curr.path {
                                        app.ui.dirPrev = d

I have found that removing the if app.ui.msg == "" condition and updating the status bar unconditionally may be undesirable. For instance try the following scenario:

  1. Start lf and trigger an error message (e.g. unknown mapping by typing some unbound key)
  2. Start a new terminal, and modify the current directory (you can touch the directory, or create a new file in it)
  3. Without switching back to lf, force it to load (you can run lf -remote 'send load', I have also gotten this to work by configuring set period 1)
  4. The error message from step 1 should be wiped out and replaced with the updated file information, even without doing anything in lf, which users could find annoying.

Solution 2:

Clear app.ui.msg instead of calling loadFileInfo in step 2, which means the if app.ui.msg == "" condition will end up being true. I think this approach is safer.

Checkout this branch: https://github.com/joelim-work/lf/tree/reload-status-bar-2

--- a/eval.go
+++ b/eval.go
@@ -1312,7 +1312,7 @@ func (e *callExpr) eval(app *app, args []string) {
                        app.ui.echoerrf("reload: %s", err)
                }
                app.ui.loadFile(app, true)
-               app.ui.loadFileInfo(app.nav)
+               app.ui.msg = ""
        case "read":
                if app.ui.cmdPrefix == ">" {
                        return

This approach keeps the if app.ui.msg == "" condition so that error messages won't get wiped out. I haven't found any downsides with this approach, other than the fact that it is kind of a hack to clear app.ui.msg just to satisfy that condition when the directory is reloaded.

@DusanLesan
Copy link

@joelim-work I have made comparison between reload-status-bar-1 (S1), reload-status-bar-2 (S2) and master on your repo and it looks to me that S1 covers most cases. Please confirm if that looks ok to you and if you need me to further explain taken actions

Action S1 S2 Master
load: clears error after no changes n n n
load: clears error after editing focused file n n n
load: refreshes status bar after editing focused file n n n
load: refreshes file preview after editing focused file y y y
load: clears error after editing other file n n n
load: clears error after making new file y n n
load: clears error after editing file in focused dir n n n
load: refreshes status bar after deleting focused dir and making new with the same name y n n
load: refreshes status bar after making file in focused dir n n n
load: clears error after making file in focused dir n n n
load: refreshes dir preview after making file in focused dir y y y
reload: clears error after no changes y y y
reload: clears error after editing focused file y y y
reload: refreshes status bar after editing focused file y y n
reload: refreshes file preview after editing focused file y y y
reload: clears error after editing other file y y y
reload: clears error after making new file y y y
reload: clears error after editing file in focused dir y y y
reload: refreshes status bar after deleting focused dir and making new with the same name y y n
reload: refreshes status bar after making file in focused dir y y n
reload: clears error after making file in focused dir y y y
reload: refreshes dir preview after making file in focused dir y y y

@joelim-work
Copy link
Collaborator

Hi @DusanLesan, that is a lot of test cases - thanks for your help.

The main reason why I didn't like Solution 1 was because of the load: clears error after making new file scenario, which is what I described previously. I also noticed that the behaviour is different for load: refreshes status bar after deleting focused dir and making new with the same name - that is because Solution 2 clears the status bar only for the reload command and not the load command, while Solution 1 updates the status bar regardless of whether it has already been cleared or not.

The difference between load and reload can be a bit confusing - from reading the code load checks every directory from the root directory to the current directory and reloads any directory that has been modified, while reload clears the data structures (including caches) and re-populates them from scratch.

Anyway, based on your testing do you think Solution 1 or 2 (preferably 2) solves the original issue? Or is there anything that you think needs addressing?

@DusanLesan
Copy link

Yes. Both of them address all the reload cases I could think of, so you can use whichever one you prefer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants