Re: [RFE][PATCH] Display free space on device in panels

[Jindrich Novy <jnovy redhat com>]

Hi Pavel,

On Tue, 2006-05-30 at 14:56 +0300, Pavel Tsekov wrote: 
> On Mon, 29 May 2006, Jindrich Novy wrote:
> 
> > On Fri, 2006-05-19 at 14:29 -0400, Pavel Roskin wrote:
> >> Can we avoid any highlighting?  I think Far Manager does it right:
> >> http://red-bean.com/proski/mc/far.png
> >
> > Attaching the patch without highlighting. The main changes are that I
> > moved the show_free_space() into main.c since screen.c lacks a header
> > file and a call is needed from cmd.c to minimize frequency of gathering
> > filesys info. The free space widget isn't displayed at all when browsing
> > a non-local fs. Filesystem statistics are updated immediately when mc
> > requests a reread (ctrl-r is pressed).
> >
> > Please review, some further mistakes are possible.
> 
> Now, after I reviewed the patch for a second time I am pretty convinced 
> that the free space info should be displayed in the mini status area
> and not on its upper frame i.e. as it is in FAR manager (what Pavel 
> Rosking suggested). 

Yes, otherwise it would be too little space for cwd, especially for
80x25 terminals.

> MC tries to optimize the screen drawing by updating only certain parts
> of the screen (the directory contents, the mini status) when not otherwise 
> necessary. Simply put MC doesn't update the whole screen when you go down 
> one file in the panel, when you change a directory, etc. With the current 
> patch you actually break that optimization by inserting a call to
> mini_info_separator() in show_dir() which is called each time you move
> through the file list for example.

Ok, the only speed related optimization I see is to not to draw the free
space widget every time the show_free_space() is called, but only in
case when old_cwd and panel_cwd differs. The trick to force redraw when
user presses ctrl-r by setting old_cwd to NULL will work then anyway.

> Your patch may be modified to so that it will work properly i.e. it wont
> break the optimization but it becomes ugly (codewise). It may be modified
> to be not so ugly by sacrificing the optimization a bit i.e. put
> a call to mini_info_separator () in panel_update_contents(). It also
> can be made to fit into the current scheme by making the free space info
> part of the mini status area.

Did you do some profiling that it slows mc down significantly? I haven't
seen a noticable delay, especially after show_free_space() doesn't draw
the widget each time it's called.

> Now, there are some other small issues with the patch as-is:
> 
> --- mc-4.6.1a/src/main.c.showfree	2006-05-29 12:41:36.000000000 +0200
> +++ mc-4.6.1a/src/main.c	2006-05-29 13:04:50.000000000 +0200
> @@ -402,6 +409,8 @@
>       int reload_other = !(force_update & UP_ONLY_CURRENT);
>       WPanel *panel;
> 
> +    show_free_space(current_panel);
> +
>       update_one_panel (get_current_index (), force_update, current_file);
>       if (reload_other)
>   	update_one_panel (get_other_index (), force_update, UP_KEEPSEL);
> 
> This part is not necessary. And it will draw the free_space_info() even
> if there is no mini status (though you won't see it).

show_free_space() checks whether free_space != 0 otherwise it won't draw
anything.

> @@ -467,6 +476,37 @@
>       }
>   }
> 
> +void
> +show_free_space(WPanel *panel)
> +{
> +    struct stat st;
> 
> This is not necessary as well as the code which fills it.

Yes, I'll remove it.

Jindrich