Bug 91052 - cleanup makeWindow constructors ...
Summary: cleanup makeWindow constructors ...
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: LibreOffice (show other bugs)
Version:
(earliest affected)
4.4.3.2 release
Hardware: Other All
: medium normal
Assignee: Not Assigned
URL:
Whiteboard: target:5.0.0
Keywords: difficultyBeginner, easyHack, skillCpp, topicCleanup
Depends on:
Blocks:
 
Reported: 2015-05-03 18:02 UTC by Michael Meeks
Modified: 2015-12-16 00:10 UTC (History)
3 users (show)

See Also:
Crash report or crash signature:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Meeks 2015-05-03 18:02:39 UTC
VclBuilder constructs a symbol name by concatenating widget names with a 'make' prefix; cf.

vcl/source/window/builder.cxx:

            OUString sFunction(OStringToOUString(OString("make") + name.copy(nDelim+1), RTL_TEXTENCODING_UTF8));

This is cast to this type:

    typedef vcl::Window* (*customMakeWidget)(vcl::Window *pParent, stringmap &rVec);

And invoked. In the new world of VclPtr's we should change this signature to:

    typedef VclPtr<vcl::Window> (*customMakeWidget)(vcl::Window *pParent, stringmap &rVec);

instead. Which is just a 1x line change. But then it is necessary to do:

git grep -Hn -3 -e 'Window *\*.*make.*('

And change every instance of every 'make' function thus:

eg. chart2/source/controller/dialogs/TextDirectionListBox.cxx:45:

extern "C" SAL_DLLPUBLIC_EXPORT vcl::Window* SAL_CALL makeTextDirectionListBox(vcl::Window *pParent, VclBuilder::stringmap &)
{
    return new TextDirectionListBox(pParent);
}

into:

VCL_BUILDER_CONSTRUCTOR makeTextDirectionListBox(vcl::Window *pParent, VclBuilder::stringmap &)
{
    return VclPtr<TextDirectionListBox>::Create(pParent);
}

where we define:

#define VCL_BUILDER_CONSTRUCTOR extern "C" SAL_DLLPUBLIC_EXPORT VclPtr<vcl::Window> SAL_CALL 

Not an elegant define, but (hopefully) will help us find all these call-sites in the future =)

Might be nice to have another parameterized macro that takes the type, and just passes the pParent directly on to a method it-inlines.

No idea; thoughts Caolan ? =)
Comment 1 Michael Meeks 2015-05-03 18:04:08 UTC
Thoughts Caolan ? - oh - and ... we need to drop the SAL_NO_ACQUIRE in the VclBuilder method after this, and also cleanup all the 'new Foo' stuff inside the VclBuilder itself (one of the last places that is there).

Read vcl/README.lifecycle to understand the ref-counting there.
Comment 2 Caolán McNamara 2015-05-07 09:41:06 UTC
could also remove all the makeFOO stuff and pass a factory helper to the builder from the dialog ctors so if you use custom widgets in a dialog/builder the caller has to send in a thing that knows how to make the custom widgets
Comment 3 Commit Notification 2015-05-09 19:31:57 UTC
Michael Meeks committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=cf6bfd666843c1cfc75e453e6e7f0405173a1831

tdf#91052 - use macros to standardize 'make' constructors for VclBuilder.

It will be available in 5.0.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds
Affected users are encouraged to test the fix and report feedback.
Comment 4 Commit Notification 2015-05-09 20:34:31 UTC
Michael Meeks committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=266ce1cd530b4ccb2f6ca8c58553da150e9ef40d

tdf#91052 - more macros for 'make' constructors.

It will be available in 5.0.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds
Affected users are encouraged to test the fix and report feedback.
Comment 5 Commit Notification 2015-05-11 12:02:47 UTC
Michael Meeks committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=b01554bcdaca076f900c25b672fd691dbe76c9db

tdf#91052 - more macros for 'make' constructors.

It will be available in 5.0.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds
Affected users are encouraged to test the fix and report feedback.
Comment 6 Commit Notification 2015-05-11 12:51:30 UTC
Michael Meeks committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=ceb6f473837261f2a6e43e028ce9da3daccc2f6c

tdf#91052 - more macros for 'make' constructors.

It will be available in 5.0.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds
Affected users are encouraged to test the fix and report feedback.
Comment 7 Michael Meeks 2015-05-11 13:53:02 UTC
I've completed this locally; but there is some follow-on work that I'll turn into another easy-hack =)
Comment 8 Robinson Tryon (qubit) 2015-12-16 00:10:25 UTC
Migrating Whiteboard tags to Keywords: (easyHack difficultyBeginner skillCpp topicCleanup )
[NinjaEdit]