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

Update directory-movement.lua #104

Closed
wants to merge 2 commits into from
Closed

Update directory-movement.lua #104

wants to merge 2 commits into from

Conversation

IC0hO
Copy link

@IC0hO IC0hO commented Apr 4, 2024

When accessing an address like "ftp://localhost", navigating forwards from there would incorrectly jump to just "ftp://", resulting in an error. The purpose of this commit is to clear the value of g.state.directory when it equals the protocol prefix, to prevent this erroneous behavior.
Screenshot 2024-04-04 155856

IC0hO added 2 commits April 4, 2024 16:12
When accessing an address like "ftp://localhost", navigating upwards would jump to "ftp://", resulting in an error. The purpose of this commit is to clear the value of g.state.directory when it equals the protocol prefix.
When accessing an address like "ftp://localhost", navigating forwards from there would incorrectly jump to just "ftp://", resulting in an error. The purpose of this commit is to clear the value of g.state.directory when it equals the protocol prefix, to prevent this erroneous behavior.
@CogentRedTester
Copy link
Owner

Can you explain what the 2nd commit you made does? Because it's not what you're describing above and in the commit messages.

@IC0hO
Copy link
Author

IC0hO commented Apr 18, 2024

The purpose of this pull request is that, in the process of going up one level, we want the configured address in the file_browser.conf to be used as the final accessed directory, instead of going up to the address itself.

Originally, if the configuration was ftp://localhost/, going up one level would attempt to access ftp://. If the configuration was ftp://localhost/movie/, going up one level would access ftp://localhost/.

Now, with the added code, if the configuration is ftp://localhost/, going up one level will directly return to the root directory. If the configuration is ftp://localhost/movie/, going up one level will also directly return to the root directory.

In the first commit, if the file_browser.conf was configured with a directory like ftp://localhost/movie/, going up one level would be ftp://localhost/, instead of the expected return to the root directory.

The second commit can correctly handle directories like ftp://localhost/movie/, and going up one level will directly return to the root directory.

Copy link
Owner

@CogentRedTester CogentRedTester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with this sort of check, but you can't use the cache to do it, see my code comment. Also before I can accept this PR you need to squash all these commits into one.

Comment on lines +42 to +44
if #cache > 0 and cache[#cache]["directory"] == "" then
g.state.directory = ""
end
Copy link
Owner

@CogentRedTester CogentRedTester Apr 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't strictly fix the issue. This condition assumes that the cache will always reliably show the parent directory, but this is not the case; there are various ways that the cache can be cleared. For example jumping to the directory for the current file will clear the cache, as will using the Ctrl+r rescan keybind. Your previous approach was better, though if you're going to detect if the path contains a protocol scheme you should use the existing API.get_protocol function.

Also if you choose to use the protocol detection method I'd like you to add a new option that disables the behaviour.

@IC0hO
Copy link
Author

IC0hO commented Apr 19, 2024

  if API.get_protocol(g.state.directory, def) then
      g.state.directory = ""
  end

Can the modification be done in the way mentioned above? My testing seems to have satisfied the requirements. After all, I don’t know much about this project, I’m just tentatively trying to meet the requirements. It would be great if you have a better solution.

CogentRedTester added a commit that referenced this pull request Apr 19, 2024
e.g. moving up from `ftp://localhost/` will move straight to the root instead of `ftp://`

First brought up in (#104)
CogentRedTester added a commit that referenced this pull request Apr 19, 2024
e.g. moving up from `ftp://localhost/` will move straight to the root instead of `ftp://`

First brought up in (#104)
@CogentRedTester
Copy link
Owner

I've made an alternate proposal in #106 that shows how I would do it.

CogentRedTester added a commit that referenced this pull request Apr 19, 2024
e.g. moving up from `ftp://localhost/` will move straight to the root instead of `ftp://`

First brought up in (#104)
@IC0hO
Copy link
Author

IC0hO commented Apr 19, 2024

I tried using these codes, and directory access returned normally. There is just one problem. Directories such as ftp://localhost/movie/ will continue to return to ftp://localhost/ instead of returning to the root directory.

@CogentRedTester
Copy link
Owner

Yes that is deliberate, the root has always worked like that. Avoiding that issue is beyond the scope of a PR like this, it would be much more complicated.

@IC0hO
Copy link
Author

IC0hO commented Apr 20, 2024

I see...
Thank you for your solution

@IC0hO IC0hO closed this Apr 20, 2024
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 this pull request may close these issues.

2 participants