shoes / shoes3

a tiny graphical app kit for ruby
http://walkabout.mvmanila.com
Other
181 stars 19 forks source link

title attribute for ask_open/save_file/folder #280

Closed passenger94 closed 8 years ago

passenger94 commented 8 years ago

i stumbled upon this, while looking at svg, what do you think ?

Also, not related , this https://github.com/Shoes3/shoes3/blob/master/shoes/svg.c#L78 looks like it is a waste of memory (60/70Mo in our case) to save that data, we have everything needed in the handle object at that point.

keep the data member in the struct, just in case, but not stashing useless data, or get rid of it ? Thoughts ? (same is valid, i think, for shoes_svg content attribute, once the handle is created, we don't need it anymore, unless we want a Shoes xml svg Editor ... - and why not :-D - )

ccoupe commented 8 years ago

we need to call free() the space allocated by strdup. on the ptr is stored in the stack, the buffer is on the heap.

passenger94 commented 8 years ago

i was thinking to not allocate anything at all (keep self_t->data NULL), we don't need it, no ?

ccoupe commented 8 years ago

man strdup - it mallocs for you but you have to free it your self. Aslo true for the strdups in the new svg fixes.

passenger94 commented 8 years ago

? do we talk about the same thing ? i'm proposing to NOT strdup anything ... :-D ... that member is useless

ccoupe commented 8 years ago

I was looking at the gtk.c code you modified above line 1788, 1789

ccoupe commented 8 years ago

You can't reuse the parameter/argument 'title' since it may have memory allocated to it whiich needs freeing before being overwritten.

passenger94 commented 8 years ago

lol ok ! i was looking at the svg_handle which happens to have a strdup also i need to add a free() call for title char* ok :-)

oh ! i thought strdup was doing the strcopy, etc .. under the hood

passenger94 commented 8 years ago

ok i think i understood about the reuse of title, funny thing is that it works ! So, if we provide a title attr, we need to free the title var, which we are sure it is allocated (it's "Open file...", "Save fie..." etc...)

and create a new char *title from the strdup and free that one too when dialog is destroyed, or is it freed by the dialog ?

passenger94 commented 8 years ago

i'm confused ... title is a pointer to a char*,so i can tell it to point to another location in memory, no ? ... ... as for freeing it later, nobody is complaining if i free it just before destroying the dialog, i'm adding it to the PR

ccoupe commented 8 years ago

There's two other issues. This breaks existing Shoes scripts that don't have the hash arg. Second, I was hoping to never touch that section of the osx code again. Do we really gain anything at the Ruby level in clarity or consistency?

ccoupe commented 8 years ago

On the issues of svg

Also, not related , this https://github.com/Shoes3/shoes3/blob/master/shoes/svg.c#L78 looks like it is a waste of memory (60/70Mo in our case) to save that data, we have everything needed in the handle object at that point.

I commited a small fix to svg.c to free the 3 strdup buffers you created when Ruby wants to gc finalize an svghandle (because nothing in Ruby is pointing to that object). That may help the memory load or not - depends on what the shoes script is doing and hanging on to. It might even be the real leak to fix.

keep the data member in the struct, just in case, but not stashing useless data, or get rid of it ? Thoughts ? (same is valid, i think, for shoes_svg content attribute, once the handle is created, we don't need it anymore, unless we want a Shoes xml svg Editor ... - and why not :-D - )

I believe @BackOrder originally wanted to 'water mark' some svgs so it would take an svg editor.

passenger94 commented 8 years ago

This breaks existing Shoes scripts that don't have the hash arg ??? Are you talking about the Open Dialog title in hash arg ?

free fix to svg.c were in the PR .... (don't need the if guard ...) nothing better in the leaks department...

ccoupe commented 8 years ago

Yes, I was confused. OSX support shouldn't be too hard