PKGSRCGFE=PKGSRC GRAPHICAL FRONT END

[Dennis Melentyev]

Hi Saverio,

It is a good idea to write a GUI frontend and written code is much
better than just an idea to write it, but I still see a long way to
go.

Please let me comment your code a little.

And (I'd like to emphasize this) it is absolutely not an attempt to
hurt you, but rather friendly general comments for future improvement
of you coding skills. Please, do not take it personally.

Current code is good for the proof-of-concept project. To make it into
"production", you'll have to work a lot on architecture and
components.
I'd suggest to freeze it to the current functionality, fix every
possible bug and start the next version of it, with following notes in
mind.

Typical problems are:
1. Heavy use of hardcoded magic constants.
2. The code is too "linear": Doing everything in main() is not the
best practice. You have to separate logic into specific functions.
Even if there is only one call for some function.
This will improve code readability and let you maintain your code more
easily in the future.
3. I hardly can see the reason for this construction:
  for (i = 1; i <= 4; i++) {
    switch (i) {
      case 1: page = gtk_vbox_new(TRUE, 0);
. ...

This saves you nothing. Just make it four separate functions and call
them in a sequence. Or, refactor code to have some common part and
some conditional part within a cycle.
Something like:
for (i=0; i<LAST_PAGE; I++) {
   /* prepare page-specific data */
  switch(i) {
    case 1:
      page = ....;
      psgfe_add_button(BUTTON_ONE);
      psgfe_add_button(BUTTON_TWO);
      psgfe_add_button(BUTTON_THREE);
      break;
    case 2:
      page = ....;
      psgfe_add_button(BUTTON_ONE_A);
      psgfe_add_button(BUTTON_TWO_A);
      psgfe_add_button(BUTTON_THREE_A);
      break;
. ..
   default:
      /* Report a missed case and kill the program */
      assert(...);
  }

   /* Put page-independent code here */
}

4. Absolutely no diagnostics. This hurts you in the first place. There
is no way to understand what happen to the user when he reports a
problem. Just consider -d or -v option in most programs.

5. Almost no error handling:
        fd = g_open("/usr/pkg/share/pkgsrcgfe/mirror.conf", O_RDONLY);
        if (fd < 0) perror("fd < 0");
        lseek(fd, 0, SEEK_SET);

perror() does not aborts execution. Even more, it does output to
strerr, but you will miss it in X environment most of the time.
lseek() could fail also. As well as read(), malloc() and many other
calls.

Also, you leak a file handle each time a mirror.conf is opened. There
are no g_close() or something like that.

6. The only comments in the code is a license block.
Be verbose in comments. This does not affect performance, but help
others _AND_ yourself to read and understand the code.

Didn't tried to dig any deeper, but hope, this friendly
suggestions/notes will help you for mutual benefit of the community.

Thanks for the efforts and good luck in this great project!

2008/11/15 dark0s Optik <shiftcoder at gmail.com>:
> I modified program and I tell you test it, especially if it download package
> in pkg_add operation and if it works.
> I have a very very slow internet connection and cannot test it fastly.
>
> The file mirror.conf must be located in /usr/pkg/share/pkgsrcgfe directory
> and contains mirror where pkg_add must to connect. If you want change
> mirror, then you must edit mirror.conf, but you write only one row without
> '\n' character.
>
> I must complete yet this program:
> 1) save button in mirror.conf window don't work, I must permits writing and
> saving mirror.conf from dialog window
> 2) I'd like write multiple mirror in mirror.conf
>
> I ask you tell me any thing don't work.
>
> Regards,
> savio
>



-- 
Dennis Melentyev