Issue 122116

Created attachment 80571 [details]
GDB backtrace

Slot SID_GALLERY_FORMATS is dispatched in 
void GalleryBrowser2::ImplExecute( sal_uInt16 nId )
main/svx/source/gallery2/galbrws2.cxx

and executed in the respective shells:

* sc: void ScTabViewShell::ExecGallery( SfxRequest& rReq )
sc/source/ui/view/tabvwsh9.cxx

* sw: void SwBaseShell::Execute(SfxRequest &rReq)
sw/source/ui/shells/basesh.cxx
(there is a potential crash here, dereferencing a null pointer: the request argument is assumed to be always present)

* sd: void DrawViewShell::ExecGallery(SfxRequest& rReq)
sd/source/ui/view/drviews9.cxx


The common work-flow is something like:

GalleryExplorer* pGal = SVX_GALLERY();
if it is a graphic, get the graphic
Graphic aGraphic = pGal->GetGraphic();
...
else if it is a media, get the URL
... = pGal->GetURL();


The root cause is how these member functions are implemented in GalleryExplorer
/svx/inc/svx/gallery.hxx
svx/source/gallery2/galexpl.cxx

INetURLObject GetURL() const;
String GetFilterName() const;
Graphic GetGraphic() const;
sal_Bool GetVCDrawModel( FmFormModel& rModel ) const;
sal_Bool IsLinkage() const;


// ------------------------------------------------------------------------

INetURLObject GalleryExplorer::GetURL() const
{
	return GALLERYBROWSER()->GetURL();
}

String GalleryExplorer::GetFilterName() const
{
	return GALLERYBROWSER()->GetFilterName();
}

// ------------------------------------------------------------------------

Graphic GalleryExplorer::GetGraphic() const
{
	return GALLERYBROWSER()->GetGraphic();
}

// ------------------------------------------------------------------------

sal_Bool GalleryExplorer::GetVCDrawModel( FmFormModel& rModel ) const
{
	return GALLERYBROWSER()->GetVCDrawModel( rModel );
}

// ------------------------------------------------------------------------

sal_Bool GalleryExplorer::IsLinkage() const
{
	return GALLERYBROWSER()->IsLinkage();
}

// ------------------------------------------------------------------------


The macro GALLERYBROWSER() 
svx/inc/svx/galbrws.hxx
#define GALLERYBROWSER() ((GalleryBrowser*)( SfxViewFrame::Current()->GetChildWindow(GalleryChildWindow::GetChildWindowId())->GetWindow()))

assumes that when the slot is executed, the gallery browser is opened and the slot has been executed for a gallery item select in it.
With the gallery inside the gallery control of the sidebar, these assumptions are wrong: when only the sidebar's gallery control is present, and the slot is dispatched from the context menu in it, it crashes: there is no gallery child window; the GalleryControl is just a Window with the GalleryBrowser1 and GalleryBrowser2.

Among the possible solutions, one could be:

* change these member functions, make them static, taking a theme name and a theme item id as argument; and
* change the slot definition (right now it is already wrong: it is dispatched with arguments, but defined without them): add slot arguments, for example: theme name, theme item id, gallery item format, if item is link, item URL (when link)

Ariel, thanks for the analysis.  I agree with your proposed solution.  Would you like to fix it?

(In reply to comment #3)
> Ariel, thanks for the analysis.  I agree with your proposed solution.  Would
> you like to fix it?

I'll work on this on the weekend.

Adapting summary: inserting a gallery item with the keyboard (*) is also broken

(*) Keys I and Insert (Ctrl + Shift, insert as link), see http://svn.apache.org/viewvc/openoffice/trunk/main/svx/source/gallery2/galbrws2.cxx?revision=1466396&view=markup#l624

Assigning issue to Ariel as he expressed to work on it.
This just helps me for my overview about the sidebar issues.

"arielch" committed SVN revision 1479765 into trunk:
i122116 - Remove GalleryExplorer member functions

Fixed on trunk

Created attachment 80669 [details]
Some test cases

Some home-made test cases.

All test cases are about the "old" Gallery docking window ("Tools" - "Gallery"). They should be repeated with only the sidebar gallery panel, and with both galleries open at the same time.

(In reply to comment #2)
> Among the possible solutions, one could be:
> 
> * change these member functions, make them static, taking a theme name and a
> theme item id as argument; and
> * change the slot definition (right now it is already wrong: it is
> dispatched with arguments, but defined without them): add slot arguments,
> for example: theme name, theme item id, gallery item format, if item is
> link, item URL (when link)

I took another approach:

* added a new SvxGalleryItem:

- the gallery item ID is a sal_uIntPtr, this is a sal_uInt32 or sal_uInt64, depending on the arch. (sal/inc/sal/types.h) There is no SfxPoolItem to hold a sal_uIntPtr, so that changing the slot definition to add a gallery item id argument, would have required to create one
- for UNO clients, dispatching the command would require to know the item ID. The fact that the index of an UNO gallery item is the same as the one of the internal implementation, is simply an implementation detail (main/svx/source/unogallery/)
Adding this new pool item had the advantage of completely remove some stuff, as described below.

* remove the member functions, and turn the GalleryExplorer into a mere utility class.

- INetURLObject GetURL() const;
- String GetFilterName() const;
- Graphic GetGraphic() const;
- sal_Bool GetVCDrawModel( FmFormModel& rModel ) const;
- sal_Bool IsLinkage() const;

* remove the define SVX_GALLERY()

#define SVX_GALLERY() (GalleryExplorer::GetGallery())

* remove the defines for gallery item type from the public interface of the GalleryExplorer; instead of it, use the IDL definition of css::gallery::GalleryItemType in the client code (sw, sc, sd)

* Moved the slot definition of SID_GALLERY_BG_BRUSH from sfx2 to svx: the method slot arguments were wrong, it also needed an SvxBrushItem

sfx2/sdi/sfx.sdi
SfxVoidItem BackgroundImage SID_GALLERY_BG_BRUSH
(SfxStringItem ImageFile SID_FILE_NAME)

svx/sdi/svx.sdi
SfxVoidItem BackgroundImage SID_GALLERY_BG_BRUSH
(SvxBrushItem Background SID_GALLERY_BG_BRUSH, SfxUInt16Item Position SID_GALLERY_BG_POS)

* avoid crash on the shells' execute method by checking the request arguments

* the context menu was retrieving the state of the slots and executing them bypassing the UNO layer, this was solved by moving all related code to only use pure UNO dispatch framework

* as a consequence of the bug described above, the gallery was inserting graphics in protected cursor (a protected cell in a table, a protected section), this was solved by the same fix

(In reply to comment #9)
> * remove the define SVX_GALLERY()
> 
> #define SVX_GALLERY() (GalleryExplorer::GetGallery())

Reminder:

Forgot to commit the removal of define GALLERYBROWSER (svx/inc/svx/galbrws.hxx)
Trunk is broken right now, so I can't update my local copy.

"arielch" committed SVN revision 1485272 into trunk:
i122116 - remove GALLERYBROWSER() macro