Re: [fltk.general] [FLTK 1.3] Filesystem list in Fl_File_Browser(STR#2935)
Michael Baeuerle wrote: I will add the test information to the STR and update the patch. Done. All - I'm looking at this patch #2935 and it looks OK, and Michael reports that it works well on the problem targets. I find it does not break my linux builds. I propose to apply this patch sometime soon'ish - hope to get to it during the course of today. Anyone have any objections? Selex ES Ltd Registered Office: Sigma House, Christopher Martin Road, Basildon, Essex SS14 3EL A company registered in England Wales. Company no. 02426132 This email and any attachments are confidential to the intended recipient and may also be privileged. If you are not the intended recipient please delete it from your system and notify the sender. You should not copy it or use it for any purpose nor disclose or distribute its contents to any other person. ___ fltk mailing list fltk@easysw.com http://lists.easysw.com/mailman/listinfo/fltk
Re: [fltk.general] [FLTK 1.3] Filesystem list in Fl_File_Browser(STR #2935)
I have tested the new code block for AIX on version 5.1 (which lacks the prototype for 'mntctl()' in the header file). I have tested the code block for NetBSD on versions 2.0 and 5.1 to check that the version detection works as intended (and old versions that don't support 'getvfsstat()' behave as before). OK, that sounds good; if you are feeling keen, might be worth recording those results in the STR in case anyone else is looking at it too? - The code doesn't seem to quite match the supposed fltk coding style, see: http://www.fltk.org/cmp.php#CS_GENERAL_CODING_STYLE for more guidelines. (I confess this is often an issue for me, the fltk style is not my default style...) Yes, the braces are not correctly placed. But this is wrong in many other places of the current code inside 'Fl_File_Browser.cxx'. I noticed that, too... To make it look consistent, should I repair this in the rest of the file too? I don't really know, but my feeling is that we should make the minimal change, so that people can more easily see what has *actually* changed. So... in that case I'd propose that you leave the existing code, even where it is wrong, but just try and make the new stuff be right. Does that sound sensible? What do others think? - Is there a possibility that the AIX branch can call free((void *) list); even if list has not been malloc'd? Would it be bad if it did? I have verified this before and initialized 'list' to NULL. 'free(NULL)' executes as NOP. This behaviour is required by POSIX and is documented in the IBM manual page too: http://pubs.opengroup.org/onlinepubs/7908799/xsh/free.html http://www.ualberta.ca/dept/chemeng/AIX- 43/share/man/info/C/a_doc_lib/libs/basetrf1/malloc.htm My intention was to make it obvious that the memory is released in any case. OK, agreed. - This is a nit-pick of mine, but I prefer: if (strcmp(/, filename) != 0) No problem, I will change this. As I say, it's a personal nit-pick of mine. I'd imagine the majority don’t' care either way! Cheers, -- Ian Selex ES Ltd Registered Office: Sigma House, Christopher Martin Road, Basildon, Essex SS14 3EL A company registered in England Wales. Company no. 02426132 This email and any attachments are confidential to the intended recipient and may also be privileged. If you are not the intended recipient please delete it from your system and notify the sender. You should not copy it or use it for any purpose nor disclose or distribute its contents to any other person. ___ fltk mailing list fltk@easysw.com http://lists.easysw.com/mailman/listinfo/fltk
Re: [fltk.general] [FLTK 1.3] Filesystem list in Fl_File_Browser(STR#2935)
Albrecht Schlosser wrote: On 12.04.2013 12:09, MacArthur, Ian (Selex ES, UK) wrote: if you are feeling keen, might be worth recording those results in the STR in case anyone else is looking at it too? Yes, please. That doesn't mean that I will be the one to look at it. ;-) [...] [Apply coding standards only to new code] Does that sound sensible? What do others think? That does exactly match my opinion. Minimal patch with correct coding style, so that we can study the patch and see what's being changed. OK, I will add the test information to the STR and update the patch. Regards, Micha ___ fltk mailing list fltk@easysw.com http://lists.easysw.com/mailman/listinfo/fltk
Re: [fltk.general] [FLTK 1.3] Filesystem list in Fl_File_Browser(STR#2935)
Michael Baeuerle wrote: I will add the test information to the STR and update the patch. Done. ___ fltk mailing list fltk@easysw.com http://lists.easysw.com/mailman/listinfo/fltk
[fltk.general] [FLTK 1.3] Filesystem list in Fl_File_Browser (STR #2935)
Hi, I have found some problems with the file chooser on AIX and NetBSD operating systems and opened STR #2935 with a patch to fix them: Selecting File systems from the menu shows either nothing (not even the root filesystem on AIX) or many entries that make no sense or are not available (like swap partitions on NetBSD). The problem is that the method 'Fl_File_Browser::load()' can't determine the mount points correctly. Unfortunately there seems to be no portable way to get this information, therefore I have added special handling code inside '#ifdef' for both cases. Up to here the first part should not affect other operating systems at all. In addition the patch adds a general default section to show at least the root filesystems if all other options have failed (and show the root filesystem consistenly as / on all Unix systems). This second part may affect other operating systems but is completely independent from the first part. Is this patch acceptable? Regards, Micha ___ fltk mailing list fltk@easysw.com http://lists.easysw.com/mailman/listinfo/fltk
Re: [fltk.general] [FLTK 1.3] Filesystem list in Fl_File_Browser (STR #2935)
I have found some problems with the file chooser on AIX and NetBSD operating systems and opened STR #2935 with a patch to fix them: Selecting File systems from the menu shows either nothing (not even the root filesystem on AIX) or many entries that make no sense or are not available (like swap partitions on NetBSD). The problem is that the method 'Fl_File_Browser::load()' can't determine the mount points correctly. Unfortunately there seems to be no portable way to get this information, therefore I have added special handling code inside '#ifdef' for both cases. Up to here the first part should not affect other operating systems at all. In addition the patch adds a general default section to show at least the root filesystems if all other options have failed (and show the root filesystem consistenly as / on all Unix systems). This second part may affect other operating systems but is completely independent from the first part. Is this patch acceptable? Hi Micha, The patch looks OK for the most part, though I have a few small comments; note also I have neither a AIX nor BSD system to actually test on, so this is just inspections really! - The code doesn't seem to quite match the supposed fltk coding style, see: http://www.fltk.org/cmp.php#CS_GENERAL_CODING_STYLE for more guidelines. (I confess this is often an issue for me, the fltk style is not my default style...) - Is there a possibility that the AIX branch can call free((void *) list); even if list has not been malloc'd? Would it be bad if it did? - This is a nit-pick of mine, but I prefer: if (strcmp(/, filename) != 0) instead of if (strcmp(/, filename)) simply because it reminds me that strcmp returns 0 for a match... I doubt most people care about that though! Cheers, -- Ian Selex ES Ltd Registered Office: Sigma House, Christopher Martin Road, Basildon, Essex SS14 3EL A company registered in England Wales. Company no. 02426132 This email and any attachments are confidential to the intended recipient and may also be privileged. If you are not the intended recipient please delete it from your system and notify the sender. You should not copy it or use it for any purpose nor disclose or distribute its contents to any other person. ___ fltk mailing list fltk@easysw.com http://lists.easysw.com/mailman/listinfo/fltk
