Hi Frank,
wow fast response !!! ( /me is not so fast and also was not is work
yesterday )
On Mon, 2008-07-28 at 13:38 +0200, Frank Schönheit - Sun Microsystems
Germany wrote:
> Hi Noel,
>
> > appeared on my radar again ;-), so I decided to try and fix it
> myself.
>
> great, I'm looking forward to have this feature.
super, I am so glad you are interested in this :-)
>
> > I do have some
> > questions/queries etc. when looking at the patch you will see my
> silly
> > comments about those
>
> The only question I found is the patch is the one about the
> SfxMedium ...
oh :-/ I thought I had 1 or 2 other todo/questiony type things in there
( but maybe I am thinking about the in progress patch I have to enable
the same for openoffice basic dialogs )
> actually, I think the usage of SfxMedium there is historically only,
> dating back to times when images where loaded asynchronously. Not sure
> if this is still the case today, but given that in a lot of cases, the
> "real" loading happens internally beforehand, then the bits are copied
> to a memory stream, and then the SfxMedium loads from the memory
> stream,
> this probably is completely obsolete.
assuming that is the case then probably that leg of the logic needs
adjusting, I am a little uncomfortable with it just 'falling' into the
test for the GraphicObject scheme and would prefer a more definitive
if( sURL.indexOf( rtl::OUString(
RTL_CONSTASCII_USTRINGPARAM("vnd.sun.star.GraphicObject:") ) == 0 ) ) further
up in the method
>
> > any I hope you aren't too unhappy with my approach and hopefully I
> can
> > fix up anything you aren't happy about, look forward to hearing some
> > comments
>
> the approach looks great - re-using the graphic cache's IDs as URL is
> a
> brilliant idea :)
well it's using the GraphicObject's id, which is generated from the
Graphic object ( that is if you don't define the ID yourself ) also
unfortunately I can't lay claim of credit to the idea :-( as uno shapes
kinda do a similar thing
> (though /me wonders whether this will stop working as soon as the
> GraphicsCache can be globally disabled)
are there plans for that? but anyway in this usage the standalone
GraphicObjects will just be inserted into the GraphicManager, I don't
think the GraphicCache ( display cache ) will come into play ( or so I
thought because we don't really use the GraphicObject(s) directly, we
just use them as a convenient container ) indeed changing the
parameters in Tools/Options/Memory ( to 1 cached object and making the
sizes minimum e.g. 1MB for display cache and object size ) and sticking
in 2 X 4Mb pics in imagecontrols into the test document ( along with the
existing controls ) seems to yield nothing but a slow load ;-)
>
> detailed comments:
>
> - setting a non-linked URL at multiple controls at once doesn't work.
> This is why the PropertyHandler API distinguishes between
> ObtainedValue and Success in the onInteractivePropertySelection
> method. The former is used to just return the value, which is then
> forwarded to all components which are being inspected.
> You should simply spare setting the ImageURL in
> impl_browseForImage_nothrow, and instead put the ObjectID into
> _out_rNewValue.
:->> good to know ( would never even have tested it and I didn't realise
it was possible to do a multiobject propertychange ) But, this will
however cause a problem because it would mean the xGrfObj ( set in
impl_browseForImage_nothrow ) will go out of scope before the imageURL
will get set ( and hence a new XGraphicObject with the same ID created )
in the toolkit. I will have to look deeper into that for alternatives
> + the documentation of css.graphic.GraphicObjectAccess/XGraphicObject
> is
> *very* sparse
sure, I was only willing to invest zero effort here as creating the uno
wrapper was
a) a last minute thing to solve a cyclic link problem ( and I only saw
the problem when doing a full clean build )
b) wasn't sure whether you would be happy with the whole approach.. but
it seems you are :-)))
So, I would be delighted to provide some documentation in the next
version of the patch
>
> - GraphicObjectAccess sounds like it should be a GraphicsCache(Access)
> - if I see this right, then its sole purpose is to convert between an
> XGraphic and the ID this graphic has in the cache - right?
not exactly, I really only think of this service as a way to generate
GraphicObject(s). Regardless of using this uno wrapper of not, when you
create a GraphicObject ( with an id ) you don't really know whether you
are creating a new object or just getting a handle to an already managed
object. When you create a GraphicObject without an id then of course you
are causing a new object to be managed. But I have no talent for naming
things, if you can suggest another name you think is more appropriate, I
will gladly substitute it
>
> - the support of the vnd.sun.star.GraphicObject URL should be
> documented
> in MediaProperties service, for the URL property (which also documents
> (some of) the other supported URL schemes)
not sure what you mean here, are we affecting MediaProperties somehow?
> + GObjectAccessImpl is not thread-safe at all, it should have an own
> mutex
Will this help any?, I did think about this ( whilst keeping in mind the
mantra "putting a mutex in any openoffice code is like playing russian
roulette" ) My conclusion was that
a) the only state mpGObject is never modified ( it is just created )
putting a mutex around it's access seemed extreme
b) ok we call [get|set]XGraphic but we are just a proxy for a real
object and just forward some calls, if a mutex should go anywhere it
probably should be in the GraphicObject itself ( but I don't know if I
would be brave enough to do that given the risk of introducing some
strange threading foo in this object )
c) remembering that you could create 1 or more XGraphicObjects with the
same id then an 'own' mutex wouldn't help much either
thinking about the above it seemed more sensible to me that we might
expect that the users of such a class might be better qualified to make
sure there was no concurrent access, as such I did assume that indeed
that would be the case when setting the properties. And I can't right
now think of a scenario with the code above where the access would be
'dangerous' Maybe using a class mutex ( i.e a class static mutex ) might
copper fasten things regarding usage from XGraphicObject but I am
unsure, I am interested in any suggestions here to make thing
better/correct here
>
> + GObjectAccessImpl ctor should also check for sId being empty
yup, I can do that, no problem
> - /me thinks the form component handler should perhaps just use
> m_eComponentClass, instead of the SupportNonLinkedImage setting in the
> component context
aha, didn't see this, lets get rid of that 'SupportNonLinkedImage' then
> - using ::svt::ImageResourceAccess::getImageXStream for a graphics URL
> sounds a little bit ... weird. At least it makes (should make) the
> casual code reader wonder if there's something wrong.
ahem yes, I think the method here is a little unfortunately named
because it will resolve any URL the GraphicProvider can manage, would
you object to renaming the class/method to something like
GraphicAccess::getImageXStream, I don't think there are many users of
the class ( /me presumes it is indeed the naming that is the problem )
And, this is a place where I did have another question/comment but not
about the naming but rather about the fact that the images are converted
to png, basically I was asking was it a problem that all images were
conferted to png and should I modify the method to allow an extra param
to be passed, the extra param could be used to indicate to use mime type
of the xGraphic retrieved from the URL rather than hardcode it to png,
the it seems I pruned the comment/question :-/ )
> + in toolkit: the handling of a Graphic-URL would belong into
> lcl_getGraphicFromURL_nothrow
you are right, that's a better place
> + in goodies: IMO, the handling of the Graphic-URL would belong into a
> dedicated method. Reasoning: a) "implLoadMemory" suggests the method
> handles memory URLs, and b) all other URL schemes have dedicated
> methods, too (implLoadResource, implLoadRepositoryImage,
> implLoadStandardImage), so this really should also hold for some
> implLoadGraphics
mmm, I thought about that when I was doing it and it seemed to me that
using GraphicObjects was in effect loading from memory also,
additionally because of implementation details of not calling
GetXGraphic on the Graphic object I wanted to keep them close ( ok a
comment would do too ) But.. anyway it's no bit deal for me to separate
it out :-)
> + goodies/source/unographic/graphicunoaccess.cxx does not compile with
> warnings=errors. Ts.
ah ok, for sure it will be clean before we finish
> + when an "embedded" graphics is used, then the respective item in the
> property browser shows the vnd.sun.sttar.GraphicObject URL - which is
> of no use at all (and confusing) to the user. In such a case, leaving
> the property display empty, or showing something like "<embedded
> graphics>" might be better
absolutely, additionally there is a nice bug where if you select an item
( a previously selected one ) in the drop down list for filenames the
image disappears ( the url looks right but it has been put to
lower-case, I am guessing it gets passed through some url parsing code
but I didn't see where and didn't look further yet )
> + bug:
> - open a new text document
> - insert an image control
> - in the property browser, add an image to the control, not linked
> => the image is displayed in the control
> - in the property browser, focus and control other than the "Graphics"
> control
> - put the focus back to the "Graphics" control
> - put the focus to some other control, again
> => the image vanishes from the control, though the vnd.sun.*-URL is
> still displayed as current "Graphics" URL
I wonder is this similar to the above, I will see.
> - close the document, discarding any changes
> => crash
now, that is bad :-(
> + bug:
> - open a new text document
> - insert an image control
> - do not assign any graphics to the image control
> - save the document
> => assertion saying something about forbidden acccess to an empty
> bitmap
does this happen for linked also?, regardless I will look and try to fix
> (non-product builds only)
> - close the document
> - open the document, again
> => three assertions about problems with the graphics
> (non-product builds only)
ok, point taken, always build with the dbg util stuff ;-), I will look
into that also.
> Thanks & Ciao
> Frank
well I hope to get back to this before the end of the week, thanks for
the useful feeback, look forward to more!!
Noel
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
