Re: Feedback for apt-history-curses-ui

[Javier Barroso]

Hello,

On Wed, Sep 9, 2015 at 7:43 PM, Reco  wrote:
>  Hi.
>
> On Wed, Sep 09, 2015 at 07:23:42PM +0200, Javier Barroso wrote:
>> Hello,
>>
>> I would like to recieve feed back about a script which I have just
>> upload to github.
>>
>> https://github.com/i5513/apt-history-gui
>>
>> It is working on my computer, but I cannot be sure it will work on
>> other debian installations.
>>
>> It is , for now a perl script which will present you dpkg.log and
>> apt/history.log in a ncurse interface.
>>
>> I'm not sure how :all architecture should be analyze. So any idea is welcome
>
>
> Off the top of my head:
>
> 1) Please replace 'gui' with 'tui' in your README. Should help to avoid
> some confusion.
Done, I have planned to make a gtk3 version

>
> 2) Please document the need of installed Curses::UI::Common and
> grep-status. My Debian installation lacks both, for example.
>
Done, thanks

> 3) At least *some* kind of help text (preferably - a manpage) would be
> welcome.
>
Done inline, for the moment

> 4) While we're at it, "apt-history.perl-curses-ui" could use explicit
> license comment. As of now, it's unclear whenever you provide this
> script on terms of GPL2 or GPL2+.
>
Added at man page

> 5) Checking for existence of 'grep-status' *before invoking* probably
> would not hurt either. Using an absolute path to 'grep-status' is a good
> idea too
>
Done

> 6) The message printed at line 93 contains a typo - 'possible' ->
> 'possibly'.
>
Thanks

> 7) The message printed at line 202 is not English as far as I can tell.
>
Thanks, fixed
> 8) This part's 'grep .' meaning currently escapes me:
>
>   open my $grep_status,  "grep-status -n -FPackage '' ".
> " -s Package -s Architecture".
> " -s Version -s Status -n |".
> "grep . |";
>
That is to erase "^$" lines, so I can play with module (%) operator

> 9) This part:
>
>   open my $hfh, '{
> zcat $(ls -rt /var/log/apt/history.log*gz)
> cat /var/log/apt/history.log;
> } |';
>
> could use proper Perl opendir handling.
>
To be done

> 10) The case of terminal with less than 15 lines is not handled at all.
>
mmm I'm not sure how to handle it

> 11) The code could use some trailing whitespace and tabulation cleanup :)
>
I will review


> Reco
>

Thank you very much!

Re: Feedback for apt-history-curses-ui

[Reco]

 Hi.

On Wed, Sep 09, 2015 at 07:23:42PM +0200, Javier Barroso wrote:
> Hello,
> 
> I would like to recieve feed back about a script which I have just
> upload to github.
> 
> https://github.com/i5513/apt-history-gui
> 
> It is working on my computer, but I cannot be sure it will work on
> other debian installations.
> 
> It is , for now a perl script which will present you dpkg.log and
> apt/history.log in a ncurse interface.
> 
> I'm not sure how :all architecture should be analyze. So any idea is welcome


Off the top of my head:

1) Please replace 'gui' with 'tui' in your README. Should help to avoid
some confusion.

2) Please document the need of installed Curses::UI::Common and
grep-status. My Debian installation lacks both, for example.

3) At least *some* kind of help text (preferably - a manpage) would be
welcome.

4) While we're at it, "apt-history.perl-curses-ui" could use explicit
license comment. As of now, it's unclear whenever you provide this
script on terms of GPL2 or GPL2+.

5) Checking for existence of 'grep-status' *before invoking* probably
would not hurt either. Using an absolute path to 'grep-status' is a good
idea too.

6) The message printed at line 93 contains a typo - 'possible' ->
'possibly'.

7) The message printed at line 202 is not English as far as I can tell.

8) This part's 'grep .' meaning currently escapes me:

  open my $grep_status,  "grep-status -n -FPackage '' ".
" -s Package -s Architecture".
" -s Version -s Status -n |".
"grep . |";

9) This part:

  open my $hfh, '{
zcat $(ls -rt /var/log/apt/history.log*gz)
cat /var/log/apt/history.log;
} |';

could use proper Perl opendir handling.

10) The case of terminal with less than 15 lines is not handled at all.

11) The code could use some trailing whitespace and tabulation cleanup :)

Reco

Feedback for apt-history-curses-ui

[Javier Barroso]

Hello,

I would like to recieve feed back about a script which I have just
upload to github.

https://github.com/i5513/apt-history-gui

It is working on my computer, but I cannot be sure it will work on
other debian installations.

It is , for now a perl script which will present you dpkg.log and
apt/history.log in a ncurse interface.

I'm not sure how :all architecture should be analyze. So any idea is welcome

Thank you

Re: Feedback for apt-history-curses-ui

[Reco]

 Hi.

On Wed, Sep 09, 2015 at 08:51:30PM +0200, Javier Barroso wrote:
> > 8) This part's 'grep .' meaning currently escapes me:
> >
> >   open my $grep_status,  "grep-status -n -FPackage '' ".
> > " -s Package -s Architecture".
> > " -s Version -s Status -n |".
> > "grep . |";
> >
> That is to erase "^$" lines, so I can play with module (%) operator

Oh, I see. I'd use "grep -v '^$'" for clarity, but they don't argue
about tastes :)


> > 10) The case of terminal with less than 15 lines is not handled at all.
> >
> mmm I'm not sure how to handle it

This:

my $packages_list = $win->add(
  "packages_list",
  "Listbox",
  -values => [@packages],
  -onselchange => \_history,
  -height => ${ENV{LINES}}-15,
  -title => "Packages (".scalar @packages.")",
  -border => "true",
  -vscrollbar => "true",
  ) || die ($!);

Since you're using $LINES explicitly, you might as well check it's
defined, has a numeric value, and a value is greater-or-equal 16.

Or scrap $LINES altogether and use an appropriate function from
Term-Size or Term-Size-Perl.


> Thank you very much!

You're welcome.

Reco