[haiku-commits] Re: haiku: hrev46746 - src/add-ons/screen_savers/icons

  • From: John Scipione <jscipione@xxxxxxxxx>
  • To: "haiku-commits@xxxxxxxxxxxxx" <haiku-commits@xxxxxxxxxxxxx>
  • Date: Fri, 24 Jan 2014 18:41:06 -0500

On Thu, Jan 23, 2014 at 4:11 AM, Axel Dörfler <axeld@xxxxxxxxxxxxxxxx> wrote:
>> On January 23, 2014 at 2:43 AM jscipione@xxxxxxxxx wrote:
>>   Iterate through each BFS volume and find vector icon data in apps
>>   using a BFS query. Cap off at 128 total icons, we could get more if
>>   desired. Default install comes with ~100 at present.
>
> What was wrong with the previous method?

What was wrong with the previous method was that it would produce
garbled bitmaps sometimes. This apparently happened sometime around
the PM merge.

I realize that my solution was incomplete as the query was only
pulling in app, not document icons. So, in hrev46756 I've reverted it
to use the MIME db again. However, it doesn't produce garbled icons
because I'm now checking to make sure that GetIcon returns B_OK before
displaying them.

I've got at least one report from kallisti5 that app icons are being
pulled from the MIME db, but, they don't for me.

I tried rebuilding my image with --use-xattr support, no luck. I'm not
sure, but, I think that there is a problem adding the vector icons to
the MIME database in the build system, at least on OS X. If I rebuild
my MIME db with `mimeset -F --all /boot/system/apps/*` then the
application icons do show up, but they don't by default. YMMV.

I'd be interested to hear if application icons show up for other
people in hrev46756 or later or not. Make sure to watch for whether or
not application icons, like Deskbar, Tracker, DriveSetup, etc. show
up, not just document icons.

>>   Rename VectorIcon to vector_icon and treat it like the POD struct that it
>>is.
>
> In the past, there has been an agreement to move away from the my_c_style 
> naming
> even for a structure like this. While I'm not quite there yet, there is 
> probably
> no reason to start renaming structs in one or the other direction.
>
> Furthermore, there is nothing wrong to allocate/delete a POD structure via
> new/delete. Using malloc/free is rather messy. Furthermore, a better change
> would have been to use a BObjectList for fVectorIcons that own its entries, so
> that they are actually freed when the screen saver is unloaded (and not leaked
> like it is now).

I implemented the BObjectList part of you suggestion.

For the new/delete part, is it more messy to use malloc or to deal
with exceptions? I guess I could use std::nothrow, but, I'd rather not
deal with the implicit constructor at all for this simple struct. The
implicit destructor is fine though since it's not gonna throw an
exception.

>> @@ -1,7 +1,12 @@
>>  /*
>> -     Copyright 2009 Vincent Duvert, vincent.duvert@xxxxxxx
>> -     All rights reserved. Distributed under the terms of the MIT License.
>> -*/
>> + * Copyright 2009 Vincent Duvert, vincent.duvert@xxxxxxx
>> + * Copyright 2014 Haiku, Inc. All rights reserved.
>> + *
>> + * Distributed under the terms of the MIT License.
>> + *
>> + * Authors:
>> + *           John Scipione, jscipione@xxxxxxxxx
>> + */
>
> I would add Vincent to the authors list as well; after all he wrote it.

No disrespect intended towards Vincent. I was under the impression
that the authors list was for authors that assigned copyright to
Haiku, Inc. only. Vincent is listed in his own copyright statement,
Copyright 2009 Vincent Duvert. But, I agree that this distinction is
confusing, so, I added him to the authors list.

>> @@ -36,16 +37,19 @@
>>  const rgb_color kBackgroundColor = ui_color(B_DESKTOP_COLOR);
>>
>>
>> +
>>  BScreenSaver* instantiate_screen_saver(BMessage* msg, image_id image)
>
> One blank line too many. Also, the "instantiate_screen_saver(...)" should go 
> to
> the next line.

thx, fixed now in hrev46756

Other related posts: