Hi Frank On Mon, 2008-08-11 at 11:25 +0200, Frank Schönheit - Sun Microsystems Germany wrote: > Hi Noel, > > thanks for your recent changes, this is going to look better and better ... Thanks for the positive feeback !! > > Two non-negligible (IMO) issues: > > + removing an embedded image from a control is possible in some > round-about way only: You need to set a linked image, and then remove > it. This is a pretty bad user experience. agreed > I think this could best be solved together with a placeholder: If we > would display a placeholder such as "<embedded graphics>", then for > the user it would be obvious (hopefully) that pressing "Del" in this > case will delete the embedded graphics from the control. ouch, yes, I took the easy route by making it blank because I wanted to avoid the localisation issue ( and not realising at all the non-deletableness that this introduces ) > However, this requires adding placeholder functionality to the > SvtURLBox, okay, I can look into that > since of course we want it to handle the placeholder text > as one big chunk only (i.e. when the user starts to type, the place > holder is completely removed). Not sure how difficult this would be. > not sure what you mean about that, probably what you mean will become obvious when I try to do it :-) > + double-clicking an image control does not allow for non-linked images. > Missed this the last time - the implementation for this is in > ImageControl.cxx, method implInsertGraphics. oh, didn't know about that > Care needs to be taken here: if the method "hasField()" returns true, > then the control is bound to a database column, in which case the > "Link" checkbox should be disabled. If it is "false", it should > be enabled (and checked by default?). I think checked by default to reflect the legacy behaviour would be consistent with what I did for the other place where the filepicker is raised, yes? > > Minor issues (which I mention only since this is an anankasm of mine > when doing code reviews :): :->> > > - sVal in FormComponentPropertyHandler::setPropertyValue is superfluous yes, I think this is a piece of partially removed debug code :-( oops > - string variables usually start with "s" (ObjectID in > FormComponentPropertyHandler::setPropertyValue) sure, consider it changed > - do you know "makeAny( <value > )"? Saves the > Any aValue( <value > ); > useValue( aValue ); > construct you often use. how can you say its a 'construct I ofter use'? afaiks I only did it once!!! :-P, however it's quite funny because generally I do use makeAny but for some reason not here, I dunno why :-/ > - The "// Bit of a hack to ..." comment in > FormComponentPropertyHandler::impl_browseForImage_nothrow is outdated. yep, I will change the comment > - isImageResourceURL and isGraphicObjectURL could be merged into a > single "isSupportedURL" or so, lowering future extension costs this > way your suggestion makes sense, I will change it > > Then, after stumbling about GraphicObjectFactory again ... what about > naming it "GraphicObject" only? It would be a) better conform to the > habit of naming the service which implements "XFoo" just "Foo" b) spare > a few letters to type :) > Of course, the constructors should be "create" and "createWithId" then. no problem for me I have no attachment the name I chose and /me is happy that for someone else to be decisive regarding such things > > Adding the same image to multiple > controls indeed adds it only once to the file yes, using the GraphicObject object, the associated url scheme and the "com.sun.star.comp.Svx.Graphic[Import|Export]Helper" service(s) used by xmloff, gets you *so much* for free. You would not believe how much code I discarded from the patch after discovering these.
Thanks, Noel as usual it may take me a while to get back to this ( I hopefully have a couple of days off the end of this week ) so it may be a little while longer --------------------------------------------------------------------- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
