Bug Hunting Session
Bug 39632 - Consolidate GetMsiProperty()
Summary: Consolidate GetMsiProperty()
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Installation (show other bugs)
Version:
(earliest affected)
unspecified
Hardware: Other Windows (All)
: medium normal
Assignee: Guy Marcus
URL:
Whiteboard: target:4.1.0
Keywords: difficultyBeginner, easyHack, skillCpp
Depends on:
Blocks:
 
Reported: 2011-07-28 09:43 UTC by Björn Michaelsen
Modified: 2017-05-21 21:53 UTC (History)
7 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 Björn Michaelsen 2011-07-28 09:43:43 UTC
=== Consolidate GetMsiProperty() ===

'''Background:''' GetMsiProperty() is being copy'n'pasted from file to file, see [http://opengrok.libreoffice.org/search?q=GetMsiProperty&project=components&defs=&refs=&path=&hist= this link]. Instead of the copy'n'paste, we should define it as an inline in a header, and use it where appropriate.

'''Skills:''' Building on Windows, simple C++
Comment 1 Florian Reisinger 2012-05-18 09:06:59 UTC
Deteted "Easyhack" from summary
Comment 2 Andras Timar 2012-05-31 00:31:28 UTC
Same should be done for GetMsiProp(), in fact GetMsiProperty() calls could be converted to GetMsiProp() calls, too.
Comment 3 Mat M 2013-01-19 17:51:52 UTC
I think we can do the same for SetMsiProperty

Since all those functions will require #include <msiquery.h>, should we replace all these with a #include "msiprop.hxx" or just the ones which are used by the inlined functions ?

And I plan to put msipro.hxx in setup_native/source/win32customactions/inc (new folder). Is it ok ?
Comment 4 Mat M 2013-01-19 22:57:49 UTC
Another advice is needed:

In the code, I found TCHAR *, LPTSTR and wchar_t *, beside string & TCHAR in code targeted y the issue.
I started to use LPTSTR for pointers, as it is standard in Windows, but am not sure about readability.
I am a cpp fresher, so any advice welcome.
Comment 5 Not Assigned 2013-01-23 11:31:07 UTC
Mathias Michel committed a patch related to this issue.
It has been pushed to "master":

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

fix for fdo#39632 : Consolidate GetMsiProperty()



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 Andras Timar 2013-01-23 12:02:02 UTC
Good job, thanks.
Comment 7 Not Assigned 2013-01-27 19:40:58 UTC
Andras Timar committed a patch related to this issue.
It has been pushed to "master":

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

Revert "fix for fdo#39632 : Consolidate GetMsiProperty()"



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 8 Andras Timar 2013-01-27 19:44:30 UTC
I'm sorry, but I had to revert your patch. It looked OK at the first glance, but after some testing it showed problems. I tested two CustonActions, DotNetCHeck and SelectLanguage, neither of them worked. Even when I uncommented the commented MessageBox calls in DotNetCheck, I got no MessageBoxes at all. Can you please have a second look at the patch? Thanks.
Comment 9 Mat M 2013-02-05 00:03:35 UTC
I tried activating Message boxes in setup_native/source/win32/customactions/shellextensions/dotnetcheck.cxx, because I think this action is always called. I did so by uncommenting lines in the extern "C" UINT __stdcall DotNetCheck(MSIHANDLE handle) function
I did it on a standard commit :
 commit 484bb96aa975d834e326d927d36ee17808b8b6b5
 Author: Tor Lillqvist <tml@iki.fi>
 Date: Sat Jan 12 15:34:28 2013 +0200
Boxes did not appear and installer complained it was interrupt before the end (although all is working fine and desktop shortcut is created.
My leads are:
1) I need to do more than just uncommenting code to activate message boxes
2) dotnetcheck custom action is not called unconditionally

If 1), could you give me some pointers, or do you have any idea on how to go further ?
If 2), As I checked in the idt files it seems to always trigger. Some pointers on how to understand idt files needed, then.
Comment 10 Björn Michaelsen 2013-10-04 18:48:05 UTC
adding LibreOffice developer list as CC to unresolved EasyHacks for better visibility.

see e.g. http://nabble.documentfoundation.org/minutes-of-ESC-call-td4076214.html for details
Comment 11 Robinson Tryon (qubit) 2015-12-14 07:14:52 UTC Comment hidden (obsolete)
Comment 12 Björn Michaelsen 2016-01-26 17:24:44 UTC
removing skillWindows keyword. There is no such thing at https://wiki.documentfoundation.org/Development/Easy_Hacks/lists/by_Required_Skill and having a "skill*" keyword makes the easyHack not even appear on that page even in "uncategorized".
Comment 13 Robinson Tryon (qubit) 2016-02-18 14:52:38 UTC Comment hidden (obsolete)
Comment 14 jani 2016-04-18 07:31:10 UTC
A polite ping, still working on this issue?
Comment 15 Stuart Swales 2016-06-17 10:35:13 UTC
This should be largely addressed by these set of fixes in the Windows installer module submitted against tdf#72606 'EasyHack _tstring/TCHAR elimination'

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

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

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

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

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

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

Note that there are a mix of ANSI/Wide calls used in the Windows installer so there isn't one single #include file defining GetMsiProperty() across the whole installer but this at least minimises the former copy'n'paste mess.
Comment 16 charan 2016-12-05 08:58:35 UTC
I want to work on this bug.
Comment 17 jani 2016-12-05 09:02:01 UTC
Have a look at this step-by-step guide:
https://wiki.documentfoundation.org/Development/GetInvolved
Comment 18 Stuart Swales 2016-12-05 12:12:58 UTC
Actually I would recommend that we consider this issue closed.

The patches I submitted (ref 2016-06-17) go 90% towards Björn's original concerns about copy-pasta; because of the remaining ANSI/Wide issues (that will be fixed as part of tdf#72606) I wouldn't try to progress this specific issue further.
Comment 19 jani 2017-01-09 08:23:44 UTC
A polite ping, still working on this issue ?
Comment 20 charan 2017-01-09 13:37:14 UTC
yes
Comment 21 jani 2017-02-09 06:53:03 UTC Comment hidden (obsolete)
Comment 22 Guy Marcus 2017-05-21 20:56:52 UTC
This looks like something I can do -- but again, not clear what the current status is from this log last updated months ago. I will assign to myself and give it a go.
Comment 23 Guy Marcus 2017-05-21 21:53:07 UTC
(In reply to Guy Marcus from comment #22)
> This looks like something I can do -- but again, not clear what the current
> status is from this log last updated months ago. I will assign to myself and
> give it a go.

Closing this as a bug per suggestion of Stewart above and the fact that GetMsiProp() and the like do not even appear to be present in the source any more. Following the link provided to opengrok in the initial post yields zero results.