RE: FL_Shared_Image::get(...) returns NULL for an exists image - [General Use]

[MacArthur, Ian (Leonardo, UK)]

> PS: please, tell me how can I to subscribes for fltkcoredev via email ?

It is a googlegroup, the same as fltkgeneral here; it is called "fltkc...@googlegroups.com", as you would expect.

You should be able to subscribe the same as for any other googlegroup.

Note that subscriptions are moderated and can take a while to process; it will not happen instantly...



Leonardo MW 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.
********************************************************************

[MacArthur, Ian (Leonardo, UK)]

> For subj:
>
>
> FL_Shared_Image::add() does qsort, and by that
> original_ images stayed in end of array images_.
> When happens calling bsearch (in find() method)
> then it will does search only for first occurence.
>
>
> In result: I have two images with same name, but
> with different sizes. When I'm trying to get
> third image with that name but with other sizes,
> I got nothing.


Is this with 1.3.4 or with 1.4.x? I think you are using 1.3.4?

Having asked that, I'm not sure I have anything useful to offer; I contrive never to hold multiple images with the same name, I always manage the different sizes (when I resize them in the code at runtime) as specific objects, so I don't really encounter this situation.

Manolo is much better than me on the image handling stuff - he may have some suggestions for you!

[Albrecht Schlosser]

On 27.09.2017 10:49 MacArthur, Ian (Leonardo, UK) wrote:
>> For subj:
>>
>>
>> FL_Shared_Image::add() does qsort, and by that
>> original_ images stayed in end of array images_.
>> When happens calling bsearch (in find() method)
>> then it will does search only for first occurence.
>>
>>
>> In result: I have two images with same name, but
>> with different sizes. When I'm trying to get
>> third image with that name but with other sizes,
>> I got nothing.
>
>
> Is this with 1.3.4 or with 1.4.x? I think you are using 1.3.4?

I don't think that this matters; I don't remember any changes WRT shared
image handling (except drawing and scaling changes by Manolo). The
storage and search algorithms should be the same.

> Having asked that, I'm not sure I have anything useful to offer; I contrive never to hold multiple images with the same name, I always manage the different sizes (when I resize them in the code at runtime) as specific objects, so I don't really encounter this situation.

Roman mentioned search failures in Fl_Shared_Image in STR #3297
http://www.fltk.org/str.php?L3297
"New implementation of Fl_Shared_Image...":

"1) It fixes false-negative bug: the find() function might not find the
image if it happens that supplied W and H corresponds to the full-sized
original (this is due to wrongly applied logistic to the search).
Although not critical, it unnecessary causes second load of the image
(and in certain situation it might also cause a memory leak)."
[end of citation]

I'm not sure if this is exactly the same case, and I remember that I
stared at the code and could not find an obvious bug, but I also
couldn't say the code was correct. It's hard to read and understand
since it does two (binary) searches. I think it's possible that the code
is faulty, but I don't know.

[K Dmitrij]

From: Albrecht

> I think it's possible that the code
> is faulty, but I don't know.

Hi Albrecht.

There are comparing started with begin of array images (see bsearch),
(in comparator `compare'), but original_ image is stayed in end of that (qsort does it)...
by that it always returns not 0 for image with same name but with other images,
because the last added image is in begin images_ array and is not original_ image:


[CODE=CPP]

//
// 'Fl_Shared_Image::compare()' - Compare two shared images...
//


int
Fl_Shared_Image::compare(Fl_Shared_Image **i0, // I - First image
                         Fl_Shared_Image **i1) { // I - Second image
  int i = strcmp((*i0)->name(), (*i1)->name());


  if (i) return i;
  else if (((*i0)->w() == 0 && (*i1)->original_) ||
           ((*i1)->w() == 0 && (*i0)->original_)) return 0; /// \note THIS IS ALWAYS NOT HAPPENS
  else if ((*i0)->w() != (*i1)->w()) return (*i0)->w() - (*i1)->w();
  else return (*i0)->h() - (*i1)->h();
}

[/CODE]

[K Dmitrij]

code for debug:

[CODE=CPP]
#include <FL/Fl.H>
#include <FL/Fl_Shared_Image.H>
#include <FL/Fl_PNG_Image.H>


Fl_Shared_Image* get_icon(const char* name, int W = 0, int H = 0) {
if (!W || !H) {
//AppSettings::get_instance().get("icon_w", W, 20);
//AppSettings::get_instance().get("icon_h", H, 20);
W = H = 20;
}

Fl_Shared_Image* ret = Fl_Shared_Image::get(name, W, H);

if (!ret) {
printf("no matches for: %s(%d, %d) \n", name ,W, H);

for (int _i = 0; _i < Fl_Shared_Image::num_images(); _i++) {
if (strcmp(name, Fl_Shared_Image::images()[_i]->name()) == 0) {
printf(" but we have found: %s(%d, %d)\n",
Fl_Shared_Image::images()[_i]->name(),
Fl_Shared_Image::images()[_i]->w(),
Fl_Shared_Image::images()[_i]->h());
}
}
printf("+++++++++++++++++++++++++++++++\n");
}

return ret;
}



static unsigned char report_32x32_png[895] = /* binary data included from report_32x32.png */
{137,80,78,71,13,10,26,10,0,0,0,13,73,72,68,82,0,0,0,32,0,0,0,32,8,6,0,0,0,
115,122,122,244,0,0,3,70,73,68,65,84,88,133,181,151,77,111,26,71,24,199,127,187,
44,176,176,100,89,219,117,141,237,38,54,196,73,165,170,47,170,122,76,62,128,
123,245,33,39,14,205,193,82,14,190,244,20,229,228,207,80,9,245,226,67,15,93,31,
28,201,113,27,165,173,191,68,15,85,171,90,113,43,161,196,169,149,23,163,184,49,
134,5,118,103,114,240,98,193,2,94,88,146,191,52,130,121,152,125,158,223,60,243,
236,12,163,16,80,165,82,185,2,220,5,210,193,223,70,209,250,250,250,111,165,82,
233,126,216,56,173,143,237,158,170,170,119,198,9,14,112,122,122,90,92,94,94,102,
119,119,247,66,8,181,143,77,23,66,48,110,179,44,75,51,77,115,115,101,101,229,
214,168,0,239,68,217,108,150,169,169,41,205,52,205,205,98,177,56,16,162,103,9,
164,148,231,223,255,58,106,162,104,177,72,0,113,61,141,105,154,36,147,73,205,
113,156,205,213,213,85,54,54,54,122,150,163,95,13,0,112,210,20,252,184,119,12,
29,64,163,200,48,46,99,53,26,168,201,20,89,11,77,10,177,89,42,149,88,91,91,235,
130,24,152,1,33,4,47,42,199,145,130,3,144,90,128,249,5,0,190,91,46,96,38,99,26,
112,123,120,0,41,113,26,181,232,0,29,18,82,118,45,237,80,0,70,92,229,171,92,6,
199,245,198,10,174,199,99,24,113,117,120,0,33,4,0,10,240,237,141,197,177,130,
183,37,165,64,12,40,165,11,223,130,65,212,239,82,3,51,16,212,73,181,202,171,163,
10,201,68,130,249,185,89,0,158,60,61,192,19,130,217,220,12,41,93,231,224,191,
67,90,173,22,51,211,211,24,198,112,59,121,207,70,36,253,130,9,182,189,199,251,
108,109,239,240,243,47,191,158,219,182,30,252,196,214,246,14,79,15,158,33,165,
100,231,225,35,182,182,119,120,252,207,191,3,253,68,206,192,132,101,177,84,40,
144,201,24,231,99,174,93,45,224,186,30,25,227,204,86,200,47,82,175,59,88,89,115,
160,159,160,148,160,161,92,46,255,0,124,51,212,211,163,107,55,159,207,127,221,
105,8,205,128,16,18,111,200,217,4,165,197,84,20,165,103,142,221,99,194,0,254,
216,47,115,240,252,85,36,128,79,151,22,88,156,155,25,15,224,243,235,121,190,184,
158,143,4,32,165,12,173,133,80,128,134,211,160,209,114,35,1,164,146,9,226,33,
167,105,40,64,249,240,5,207,35,30,74,75,31,229,152,155,158,28,13,192,243,186,
247,254,107,151,103,249,120,97,62,18,128,231,121,61,254,66,1,130,25,120,83,173,
225,52,155,145,0,50,105,29,61,145,24,13,32,72,252,186,90,227,245,73,180,99,121,
22,133,120,108,196,26,8,2,228,38,46,145,155,184,20,9,160,159,191,80,128,224,18,
252,95,173,83,107,68,91,130,172,161,147,214,147,163,1,184,110,247,43,231,10,65,
43,218,70,136,235,137,30,127,161,0,193,19,203,76,37,48,83,23,23,210,69,10,251,
79,241,222,238,5,195,74,3,176,109,187,221,87,165,148,123,97,7,72,84,73,41,247,
109,219,86,1,1,80,44,22,81,108,219,78,3,159,0,115,192,135,192,7,186,174,127,169,
40,202,164,148,82,247,159,213,57,187,172,14,155,49,1,212,0,7,64,81,20,7,56,174,
215,235,191,3,71,192,75,224,16,248,91,3,44,224,38,240,153,15,97,58,142,99,114,
150,29,29,72,116,4,86,3,253,126,129,155,254,231,100,71,223,1,114,190,255,55,126,
240,63,129,67,205,239,124,239,59,110,183,246,108,211,62,68,91,170,15,60,232,70,
229,2,199,126,224,182,28,63,27,237,172,52,59,219,91,243,93,174,52,74,186,48,118,
0,0,0,0,73,69,78,68,174,66,96,130};



// fltk-config --use-images --compile shared_images.cxx && ./shared_images
int main() {

fl_register_images();

Fl_PNG_Image image("report", report_32x32_png, sizeof(report_32x32_png)); // original image

Fl_Shared_Image *image16x16 = get_icon("report", 16, 16); // image for menu item
Fl_Shared_Image *image20x20 = get_icon("report", 20, 20); // image for panel
Fl_Shared_Image *image24x24 = get_icon("report", 24, 24); // image for panel with growed sizes

return 0;
}

[/CODE]

[MacArthur, Ian (Leonardo, UK)]

I don't know what the "expected" (or documented) behaviour is: it's not a way I have ever used the shared image class.

I extended KDiman's sample program (as attached below) to see what happens.


So, what I found is that this loads a single png image, at 32x32 it seems.

It then query's for that image at different sizes.

In my test, it finds that image at sizes 16x16 and 32x32, but not at 20x20 or at 24x24.

Based on the doc section that Manolo posted:

> This is exactly what the doc of Fl_Shared_Image::find(const char *name,
> int W=0, int H=0) predicts:

> Finds a shared image from its name and size specifications.
> If the image name exists with the exact width W and height H, then it
> is returned.
> If W == 0 and the image name exists with another size, then the
> original image with that name is returned.


So, based on what I interpret that text as saying, I understand why it would find an image at 32x32, and why it *does not* find any image at 20x20 or 24x24.

I do not understand why it finds an image at 16x16.

Here's the console output from the run:


Query for: report(16, 16)
OK - found: report(16, 16)

Query for: report(20, 20)
no matches for: report(20, 20) in 2 images
but we have found: report(16, 16)
but we have found: report(32, 32)
+++++++++++++++++++++++++++++++

Query for: report(24, 24)
no matches for: report(24, 24) in 2 images
but we have found: report(16, 16)
but we have found: report(32, 32)
+++++++++++++++++++++++++++++++


I have no idea if that's what we expect or not, but it finds an image at 16x16 and 32x32, and not at 20x20 or 24x24.



/// Here's my tweaked version of the code ///

#include <FL/Fl.H>
#include <FL/Fl_Double_Window.H>
#include <FL/Fl_Box.H>
#include <FL/Fl_Shared_Image.H>
#include <FL/Fl_PNG_Image.H>

static Fl_Shared_Image* get_icon(const char* name, int W = 0, int H = 0)
{

printf("\nQuery for: %s(%d, %d) \n", name ,W, H);

if (!W || !H)
{

W = H = 20;
}

Fl_Shared_Image* ret = Fl_Shared_Image::get(name, W, H);

if (!ret)
{

printf("no matches for: %s(%d, %d) in %d images\n", name ,W, H, Fl_Shared_Image::num_images());

for (int _i = 0; _i < Fl_Shared_Image::num_images(); _i++)
{
if (strcmp(name, Fl_Shared_Image::images()[_i]->name()) == 0)
{
printf(" but we have found: %s(%d, %d)\n",
Fl_Shared_Image::images()[_i]->name(),
Fl_Shared_Image::images()[_i]->w(),
Fl_Shared_Image::images()[_i]->h());
}
}
printf("+++++++++++++++++++++++++++++++\n");
}

else
{
printf ("OK - found: %s(%d, %d)\n", name ,W, H);

int main(int argc, char **argv)
{
fl_register_images();

Fl_Box *boxes[4];
Fl_Double_Window *main_win = new Fl_Double_Window(600, 300, "Test Icons");
main_win->begin();

for (int idx = 0; idx < 4; ++idx)
{
int XX = 20 + (idx * 70);
int YY = 20;
boxes[idx] = new Fl_Box (XX, YY, 64, 64);
boxes[idx]->box(FL_BORDER_BOX);
}
main_win->end();

Fl_PNG_Image image("report", report_32x32_png, sizeof(report_32x32_png)); // original image

Fl_Shared_Image *image16x16 = get_icon("report", 16, 16); // image for menu item
Fl_Shared_Image *image20x20 = get_icon("report", 20, 20); // image for panel
Fl_Shared_Image *image24x24 = get_icon("report", 24, 24); // image for panel with growed sizes

if (image16x16) boxes[0]->image(image16x16);
if (image20x20) boxes[1]->image(image20x20);
boxes[2]->image(image);
if (image24x24) boxes[3]->image(image24x24);

main_win->show (argc, argv);

return Fl::run();
}

/* end of file */

[Albrecht Schlosser]

On 28.09.2017 11:25 MacArthur, Ian (Leonardo, UK) wrote:
> I don't know what the "expected" (or documented) behaviour is: it's not a way I have ever used the shared image class.

Dmitrij's demo is using Fl_Shared_Image::get(), and parts of the
documentation say:

"If the image exists, but only with another size, then a new copy with
the requested size (width W and height H) will be created as a resized
copy of the original image. The new image is added to the internal list
of shared images."
<http://www.fltk.org/doc-1.3/classFl__Shared__Image.html#abce45e743b296f7ed959d90cad1af110>

Hence, the demo program *should* always get() all requested images by
finding and resizing the _original_ image.

> I extended KDiman's sample program (as attached below) to see what happens.

And so did I.

> So, what I found is that this loads a single png image, at 32x32 it seems.
>
> It then query's for that image at different sizes.
>
> In my test, it finds that image at sizes 16x16 and 32x32, but not at 20x20 or at 24x24.

With some minor variations of the search order, adding another
(unrelated) image, or release()ing one of the images before searching
the next one, I could manage to find all images successfully.

This indicates that the binary search in Fl_Shared_Image::find() and
hence Fl_Shared_Image::get() is indeed faulty. Currently I have a
suspect and I'm testing...

> Based on the doc section that Manolo posted:
>
>> This is exactly what the doc of Fl_Shared_Image::find(const char *name,
>> int W=0, int H=0) predicts:
>
>> Finds a shared image from its name and size specifications.
>> If the image name exists with the exact width W and height H, then it
>> is returned.
>> If W == 0 and the image name exists with another size, then the
>> original image with that name is returned.
>
>
> So, based on what I interpret that text as saying, I understand why it would find an image at 32x32, and why it *does not* find any image at 20x20 or 24x24.
>
> I do not understand why it finds an image at 16x16.

First of all, the documentation cited above is that of
Fl_Shared_Image::find(), but Dmitrij is using Fl_Shared_Image::get().

The latter does two searches:
(1) with exact W and H, and if not found:
(2) with W = H = 0

Hence Fl_Shared_Image::get() /should/ always find the original image in
the demo program, but it definitely does not in /some/ cases.

Ian, thanks for your tweaked version, BTW. I'll check if I can use some
of it for my tests.

Here is the relevant part of my modified test program:

int main() {

fl_register_images();

Fl_PNG_Image image("report1", report_32x32_png,
sizeof(report_32x32_png)); // original image
Fl_PNG_Image image2("report0", report_32x32_png,
sizeof(report_32x32_png)); // original image

Fl_Shared_Image *image16x16 = get_icon("report1", 16, 16); // image
for menu item
Fl_Shared_Image *image20x20 = get_icon("report1", 20, 20); // image
for panel

// image20x20->release();

Fl_Shared_Image *image24x24 = get_icon("report1", 24, 24); // image

for panel with growed sizes

list_images(__LINE__);

return 0;
}

In the following output the last number in [] is the 'original'
attribute of the image which is protected. I added an accessor to the
lib to see what's going on:

found image: report1(16, 16) [0]
+++++++++++++++++++++++++++++++
no match for: report1(20, 20)
but we have found: report1(16, 16) [0]
but we have found: report1(32, 32) [1]
+++++++++++++++++++++++++++++++
no match for: report1(24, 24)
but we have found: report1(16, 16) [0]
but we have found: report1(32, 32) [1]
+++++++++++++++++++++++++++++++

-----------------------------
[116] Fl_Shared_Image::num_images() = 3
[ 0] report0 (32, 32), original: 1
[ 1] report1 (16, 16), original: 0
[ 2] report1 (32, 32), original: 1
-----------------------------

Neither 20x20 nor 24x24 is found in this test. The final block is from
my list_images() function and shows that only three images are loaded.

New test: change only the unrelated image name "report0" to "report2"
and run again:

found image: report1(16, 16) [0]
+++++++++++++++++++++++++++++++
found image: report1(20, 20) [0]
+++++++++++++++++++++++++++++++
found image: report1(24, 24) [0]
+++++++++++++++++++++++++++++++

-----------------------------
[116] Fl_Shared_Image::num_images() = 5
[ 0] report1 (16, 16), original: 0
[ 1] report1 (20, 20), original: 0
[ 2] report1 (24, 24), original: 0
[ 3] report1 (32, 32), original: 1
[ 4] report2 (32, 32), original: 1
-----------------------------

IMO this is definitely a bug because arbitrary loading and (as I tested
before, also releasing) of unrelated images changes the behaviour of
Fl_Shared_Image::find() and Fl_Shared_Image::get() in an unpredictable way.

AFAICT as of now this is caused by the way the list of Fl_Shared_Image's
is built *and* how the binary search in this list works. I think I found
the culprit, but I'm still researching and trying to find a solution...

[Albrecht Schlosser]

On 28.09.2017 12:31 Albrecht Schlosser wrote:
>
> IMO this is definitely a bug because arbitrary loading and (as I tested
> before, also releasing) of unrelated images changes the behaviour of
> Fl_Shared_Image::find() and Fl_Shared_Image::get() in an unpredictable way.
>
> AFAICT as of now this is caused by the way the list of Fl_Shared_Image's
> is built *and* how the binary search in this list works. I think I found
> the culprit, but I'm still researching and trying to find a solution...

Details:

The list of Fl_Shared_Images is strictly ordered by name, width, and
height, in this order of precedence. Fl_Shared_Image::get() calls
Fl_Shared_Image::find() internally. This is done as documented:

step 1: exact match with (W, H)
step 2: if not found, again with W = H = 0.

According to the documentation, step 2 should find the original image,
but this is not always the case. Consider this list of shared images:

report1 (16, 16), original: 0, ref: 1
report1 (20, 24), original: 0, ref: 1
report1 (24, 24), original: 0, ref: 2
report1 (32, 32), original: 1, ref: 10
report1 (48, 48), original: 0, ref: 1
report1 (64, 48), original: 0, ref: 1

The list of images is searched for a match with a binary search
(bsearch()) and Fl_Shared_Image::compare() as the comparison function.
Since the list is ordered by W and H an exact match of W and H will
always find the correct image if it exists (step 1).

However, a search with W = H = 0 is not guaranteed to hit the original
image (W = H = 32) since the binary search may skip the correct width
and height values and never match. In some cases the binary search may
succeed because it hits the original image by accident.

This is obviously the status quo and consistent with the findings we
made with Dmitrijs demo program.


A possible solution: As said above, the exact match is not an issue. We
must make sure that the original image is always found in a binary
search with W = H = 0 (or, as documented, only W = 0 !). My draft with
comments below uses temporary variables i0w, i0h, i1w, and i1h to make
the code more readable than using the comparisons directly. The
comparison is done in three steps:

(1) image name (as before)
(2) test for exact match of w() and h() of images
(3) comparison with variables i0w, i0h, i1w, and i1h

The temporary variables i0w, i0h, i1w, and i1h are set to zero if an
image is marked 'original_'. This makes sure that:

(a) the list is ordered with the 'original' image first
(b) the binary search with W = H = 0 always hits the original image


Draft of Fl_Shared_Image::compare():

int
Fl_Shared_Image::compare(Fl_Shared_Image **i0, // I - First image
Fl_Shared_Image **i1) { // I - Second image

// compare name

int i = strcmp((*i0)->name(), (*i1)->name());
if (i) return i;

// test for exact match first
if ((*i0)->w() == (*i1)->w() && (*i0)->h() == (*i1)->h())
return 0;

// compare width and height with special consideration of an image
// searched with w = h = 0 that matches the original image

// Implementation note:
// Use internal helper variables to set the width and height of
// original images (marked original_) to zero for later comparison.
// This ensures that the original image is always the first image in
// the list of different images with the same name since comparison
// of the original image (w = h = 0) with any other image (w > 0 && h
> 0)
// always yields 'less than'.
// If the image searched for has w = h = 0 (in the second step, as
// described above) the original image will always be found in a
// *binary* search since it is the first image in the list of a given
name.

int i0w = (*i0)->original_ ? 0 : (*i0)->w();
int i0h = (*i0)->original_ ? 0 : (*i0)->h();

int i1w = (*i1)->original_ ? 0 : (*i1)->w();
int i1h = (*i1)->original_ ? 0 : (*i1)->h();

if (i0w != i1w) return i0w - i1w;
if (i0w == 0 && i1w == 0) return 0; // original: only width needs to
match
return i0h - i1h;
}


Special note: the line

if (i0w == 0 && i1w == 0) return 0; // original: only width needs to
match

takes care of the documentation that states that an image with W = 0
matches the original image. According to this H can be any value
(unspecified), but I think (off the top of my head) that the old code
would only match if both W and H are zero. We may need to change the
documentation or the code or both to be consistent...


My tests showed success in all test cases, so I'm confident that the
code is correct, but I'm asking for peer review and testing.

I attach shared_image_compare.patch for easier testing. This patch is
for FLTK 1.4.0 - use with patch -p1.

I also attach my modified test program. Use it for testing if you like,
but note that I added another tiny patch to FL/Fl_Shared_Image.H:

In the 'public' section, add:

int original() { return original_; }


Feedback would be very much appreciated. If the new compare() function
had any unwanted side effects or bugs this would be really bad.

TIA for testing, particularly by Dmitrij who seems to have a real test
case for this.


PS: If others think this is correct I will add an STR with reference to
this thread and commit...

[MacArthur, Ian (Leonardo, UK)]

> On 28.09.2017 12:31 Albrecht Schlosser wrote:
> >

> > IMO this is definitely a bug because arbitrary loading and (as I
> tested
> > before, also releasing) of unrelated images changes the behaviour of
> > Fl_Shared_Image::find() and Fl_Shared_Image::get() in an
> unpredictable way.

I think the code looks OK - though I'd use a lot more braces "(...)" in the conditional tests...


A question, though: How many entries do people have in the image list?


Maybe I'm atypical, but I usually only have a few.
In which case, using bsearch() at all looks suspiciously like a premature optimization to me.

A brute-force check or a lsearch() would not skip the original_ image, and if there are only a small number of images to test then it'll not take much longer than the bsearch() (might even be quicker as the logic is simpler...)

And even if it is slower, how often is it called? If it is called infrequently then...

There are many other places where we just use a simple search, because they are "fast enough" so I wonder why we did something more complex (and, as it turns out, fragile) here?


Just thinking out loud...
--
Ian

[Albrecht Schlosser]

On 28.09.2017 16:53 MacArthur, Ian (Leonardo, UK) wrote:
>> On 28.09.2017 12:31 Albrecht Schlosser wrote:
>>>
>>> IMO this is definitely a bug because arbitrary loading and (as I
>> tested
>>> before, also releasing) of unrelated images changes the behaviour of
>>> Fl_Shared_Image::find() and Fl_Shared_Image::get() in an
>> unpredictable way.
>
> I think the code looks OK - though I'd use a lot more braces "(...)" in the conditional tests...

I thought about it, but ... too lazy.

I can add a few though.

> A question, though: How many entries do people have in the image list?

Who knows?

> Maybe I'm atypical, but I usually only have a few.
> In which case, using bsearch() at all looks suspiciously like a premature optimization to me.

May be, I don't know...

> A brute-force check or a lsearch() would not skip the original_ image, and if there are only a small number of images to test then it'll not take much longer than the bsearch() (might even be quicker as the logic is simpler...)

An ordered list would likely still be adequate - or maybe not, I don't
know. In case we have an ordered list we'd have to sort (qsort?) as it
is now, so we'd still need compare() ...

> And even if it is slower, how often is it called? If it is called infrequently then...
>
> There are many other places where we just use a simple search, because they are "fast enough" so I wonder why we did something more complex (and, as it turns out, fragile) here?

I really don't know. This code is perhaps older than my participation as
a FLTK user (and much older than being a developer).

> Just thinking out loud...

Yep. But if it's fixed with my changes, shouldn't we stay with bsearch()
since someone may have lots of images?

[imm]

On 28 September 2017 at 16:35, Albrecht Schlosser wrote:
>
> Yep. But if it's fixed with my changes, shouldn't we stay with bsearch()
> since someone may have lots of images?
>

I think your fix is correct, and using that is less change to the
codebase overall than ripping out the bsearch() etc. would be.

So yes, I think that is the way to go.

Do we backport to 1.3.x? I guess not, but...

[K Dmitrij]

Your changes has no the unwanted side effects !
I have tested it right now: all 83 icons loaded/resized ok! (~ 25 from them are original_)

[Albrecht Schlosser]

On 29.09.2017 08:07 K Dmitrij wrote:
> Your changes has no the unwanted side effects !
> I have tested it right now: all 83 icons loaded/resized ok! (~ 25 from them are original_)

Thanks for testing and feedback and giving us the numbers of icons
you're using.

@Ian: 83 sounds like qualifying for a binary search, doesn't it?

[Albrecht Schlosser]

On 28.09.2017 23:13 imm wrote:
> On 28 September 2017 at 16:35, Albrecht Schlosser wrote:
>>
>> Yep. But if it's fixed with my changes, shouldn't we stay with bsearch()
>> since someone may have lots of images?
>
> I think your fix is correct, and using that is less change to the
> codebase overall than ripping out the bsearch() etc. would be.
>
> So yes, I think that is the way to go.

Okay, thanks for that.

> Do we backport to 1.3.x? I guess not, but...

I'm not sure:

(1) It's a serious bug, at least if you're loading images from memory as
Dmitrij did. I think Roman's comment (on the STR mentioned above) is
correct though that it is (likely) benign in the case of loading images
from a file because it will load the image again, which is wasteful, but
doesn't really harm (WRT program behavior).

(2) It's somewhat "dangerous" if there are still (maybe other) hidden
bugs. After all I've thought about it and tested I *believe* it is
correct, but shall we take the risk?

WRT backporting to 1.3.x in general: we released 1.3.4-2 for a small but
necessary improvement of macOS. There are already some commits in
branch-1.3 that may eventually become 1.3.5, so there are chances to add
this fix to 1.3.5. Opinions?

[Manolo]

On Friday, 29 September 2017 13:08:02 UTC+2, Albrecht Schlosser wrote:

I'm not sure:

(1) It's a serious bug, at least if you're loading images from memory as
Dmitrij did. I think Roman's comment (on the STR mentioned above) is
correct though that it is (likely) benign in the case of loading images
from a file because it will load the image again, which is wasteful, but
doesn't really harm (WRT program behavior).

(2) It's somewhat "dangerous" if there are still (maybe other) hidden
bugs. After all I've thought about it and tested I *believe* it is
correct, but shall we take the risk?

WRT backporting to 1.3.x in general: we released 1.3.4-2 for a small but
necessary improvement of macOS. There are already some commits in
branch-1.3 that may eventually become 1.3.5, so there are chances to add
this fix to 1.3.5. Opinions?

I believe Albrecht's patch is correct and I see no  reason to consider risky

committing it,  in 1.4 as in 1.3.5.

This alternative form of the same algorithm may be more self explanatory:

int Fl_Shared_Image::compare(Fl_Shared_Image **i0,   Fl_Shared_Image **i1)  {

  // compare names

  int i = strcmp((*i0)->name(), (*i1)->name());
  if (i) return i;
 
  // test for exact match first
  if ((*i0)->w() == (*i1)->w() && (*i0)->h() == (*i1)->h())
    return 0;

  // image with 0 width matches image marked original
  if ((*i0)->original_ && (*i1)->w() == 0) return 0;
  if ((*i1)->original_ && (*i0)->w() == 0) return 0;
  // sort original image before others of same name
  if ((*i0)->original_) return -1;
  if ((*i1)->original_) return 1;
  // sort remaining cases by width and then by height
  int diff = (*i0)->w() - (*i1)->w();
  return diff ? diff : (*i0)->h() - (*i1)->h();
  }

[Albrecht Schlosser]

On 01.10.2017 17:41 Manolo wrote:
>
> On Friday, 29 September 2017 13:08:02 UTC+2, Albrecht Schlosser wrote:
>
> I'm not sure:
>
> (1) It's a serious bug, at least if you're loading images from
> memory as
> Dmitrij did. I think Roman's comment (on the STR mentioned above) is
> correct though that it is (likely) benign in the case of loading images
> from a file because it will load the image again, which is wasteful,
> but
> doesn't really harm (WRT program behavior).
>
> (2) It's somewhat "dangerous" if there are still (maybe other) hidden
> bugs. After all I've thought about it and tested I *believe* it is
> correct, but shall we take the risk?
>
> WRT backporting to 1.3.x in general: we released 1.3.4-2 for a small
> but
> necessary improvement of macOS. There are already some commits in
> branch-1.3 that may eventually become 1.3.5, so there are chances to
> add
> this fix to 1.3.5. Opinions?
>
>
>
> I believe Albrecht's patch is correct and I see no  reason to consider risky
> committing it,  in 1.4 as in 1.3.5.

Thanks for taking the time to review.

> This alternative form of the same algorithm may be more self explanatory:
>
> int Fl_Shared_Image::compare(Fl_Shared_Image **i0,   Fl_Shared_Image
> **i1)  {
>   // compare names
>   int i = strcmp((*i0)->name(), (*i1)->name());
>   if (i) return i;
>
>   // test for exact match first
>   if ((*i0)->w() == (*i1)->w() && (*i0)->h() == (*i1)->h())
>     return 0;
>   // image with 0 width matches image marked original
>   if ((*i0)->original_ && (*i1)->w() == 0) return 0;
>   if ((*i1)->original_ && (*i0)->w() == 0) return 0;
>   // sort original image before others of same name
>   if ((*i0)->original_) return -1;
>   if ((*i1)->original_) return 1;
>   // sort remaining cases by width and then by height
>   int diff = (*i0)->w() - (*i1)->w();
>   return diff ? diff : (*i0)->h() - (*i1)->h();
>   }

Thanks for this reworked patch. I agree that your patch suits better
because it doesn't need the additional comments I added while developing
my first version and is more self explanatory.

I modified it slightly: added braces, as suggested by Ian, removed local
variable i (replaced by diff), and other minor changes.

I opened STR #3410 [1] for documentation and added the demo program [2]
and patches for FLTK 1.3.5 [3] and 1.4.0 [4]. The only difference
between these patches is the copyright year.

[1] http://www.fltk.org/str.php?L3410
[2] http://www.fltk.org/strfiles/3410/shared_image.cxx
[3] http://www.fltk.org/strfiles/3410/compare.diff
[4] http://www.fltk.org/strfiles/3410/compare_1.4.diff

Note that the demo program shows one intentional failure for
Fl_Shared_Image::find() (not get()) and may crash if used with the
unpatched library versions because of a release() call with a NULL pointer.


I propose to commit both patches for use in FLTK 1.3.5 and 1.4.0.

+1 for both patches.

Please vote for confirmation.

[Ian MacArthur]

On Mon Oct 02 2017 14:49:55, Albrecht Schlosser wrote:

I propose to commit both patches for use in FLTK 1.3.5 and 1.4.0.

+1 for both patches.

Please vote for confirmation.

I guess I’m +1 on this.

[Manolo]

On Mon Oct 02 2017 14:49:55, Albrecht Schlosser wrote:

I propose to commit both patches for use in FLTK 1.3.5 and 1.4.0.

+1 for both patches.

Please vote for confirmation.

I also vote +1 on this.

[K Dmitrij]

If I have right to vote, then my +1 too ))

____________________________
From: Manolo

--
You received this message because you are subscribed to the Google Groups "fltk.general" group.
To unsubscribe from this group and stop receiving emails from it, send an email to fltkgeneral...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Albrecht Schlosser]

On 05.10.2017 15:48 K Dmitrij wrote:
> If I have right to vote, then my +1 too ))

Every vote is appreciated, but only devs' votes count for our internal
decisions. Usually such votes would be asked for in fltk.coredev anyway.


Dmitrij, just to be sure: did you test (again) with the modified
patch(es) posted to STR #3410? Although we believe that these patches
are correct, confirmation by you (using your app with 83 icons) would be
very helpful and appreciated. TIA.

[K Dmitrij]

Hi.

Sorry for delayed the test new patch from STR #3410: All works ok !

But: I have old version of FLTK - fltk-1.3.x-r10036, and patching was rejected by `patch -p0 -i compare.diff`...

I did copy some strings with prefix `+' and pasted them into body of Fl_Shared_Image::compare() ...

---

From Albrecht Schlosser

Subject: Re: [fltk.general] Re: FL_Shared_Image::get(...) returns NULL for an exists image - [General Use]

 

--
You received this message because you are subscribed to the Google Groups "fltk.general" group.
To unsubscribe from this group and stop receiving emails from it, send an email to fltkgeneral...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Google Groups groups.google.com Google Groups allows you to create and participate in online forums and email-based groups with a rich experience for community conversations.