Re: [Nano-devel] [PATCH] browser: merging opendir() in to one

[Benno Schulenberg]

Hello Rishabh,

Thanks for the patch.  But... oof... too many changes.

And already quickly I see you've changed the behavior:
currently, if opendir() fails before do_browser() is called,
nano only beeps and says nothing.  Your patch make it show
an error message then.  It shouldn't do that.

> All instances of opendir() - 3 in do_browser() and 1 in
> do_browse_from() - are merged, melted into one. However, I couldn't
> create absolutely single instance of opendir() as if target path
> fails, older path has to be opened.

But... who garantuees that you can reopen the older path?
Maybe it has disappeared.  So your second version is better
in that respect -- but a big nonono for the goto; you would
have to solve that in a different way.

> 1. 'kbinput = KEY_WINCH' is to keep the content of window when window
> size changes, right?

Yes.  More precisely: to redisplay the list so it will fit in a
(possibly) resized window.

> 2. I have removed newline between goto statements and preceding
> statements as they look like single logical unit:
> make-preparation-and-goto-read_browser_directory.

Details.  If I don't like, I will change it.

> 3. This one is in context to patch
> "merging-opendir-slimmer-but-crude.patch". In last moment, I realized
> re-entering in previous directory is redundant when we encounter an
> error. Previous directory is already open,

Not open in the sense of open, because it has been closed
by browser_init(), but "open" in the sense that its contents
are already present in filelist[], yes.

> we only need to wait for
> options. Is that flow better? We would need to remove. at least, one
> goto statement, right?

Don't know which goto you mean.

First some details about the patch.

After say_there_is_no_help(); there should be no
goto read_directory_contents; -- you only need to refresh
the list when an actual help text has been shown.

+    dir = opendir(path);
+    if (dir == NULL) {
+       statusbar(_("Error reading %s : %s"), target_dir, [...]

You try to open path, but when it fails you talk about target_dir.
Is path just a copy of the latter?  If so, just use one of them.

And... is old_path not just the same as present_path?

In short: too many variables.

Also, you never free old_path.


Start over, and do this one step at a time.  First just move the
opendir() from do_browse_from() to do_browser(), and in the
process remove the then superfluous paramenter 'dir'.  Also
move the closedir() from browser_init() to do_browser(), so
that all opening and closing happens in the same function.

First get that right, in one patch, without changing any behavior.

Second, do the merge of the other opendir()s into one; you
will probably have to keep the first one separate, because it
behaves different and should not blank the statusbar and such.

Benno

-- 
http://www.fastmail.com - mmm... Fastmail...