Bug 84846 - reduce copy/paste coding ...
Summary: reduce copy/paste coding ...
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Impress (show other bugs)
Version:
(earliest affected)
4.3.0.2 rc
Hardware: Other All
: medium normal
Assignee: melikeyurtoglu
URL:
Whiteboard: ToBeReviewed
Keywords: difficultyBeginner, easyHack, skillCpp, topicCleanup
Depends on:
Blocks:
 
Reported: 2014-10-09 16:40 UTC by Michael Meeks
Modified: 2017-02-14 08:57 UTC (History)
2 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 2014-10-09 16:40:59 UTC
I was just reading & translating comments in:

sd/source/ui/docshell/docshel4.cxx

and saw lots of cut/paste to remove search for:

            /********************
             * Skip to the page *
             ********************/

It'd be great to share the code there =)

Thanks !
Comment 1 Boris Egorov 2014-10-16 20:10:38 UTC
I'd like to fix this one.

Looks like DrawDocShell::GetObjectIsMarked() and DrawDocShell::GotoTreeBookmark() is almost identical (even the tracing code with method name is the same). I think we can move this code to a new private method (any idea how to name it?) and call it from the old ones. Some bool argument can be used to determine behaviour needed by one of the methods: should we mark object and which boolean to return: bFound or bUnMark.

Resulted method would still be huge (~100 lines) and can be reduced further. For example, there are repeating code for taking page number and setting edit mode (DrawDocShell::GotoBookmark() and both aforementioned methods).
Comment 2 Michael Meeks 2014-10-16 20:30:11 UTC
Sounds like a great plan =) re-factor away - I look forward to seeing your patch.
Its perhaps a hard area to unit test, but always good to have tests.
Also - if you can split your work into a series of smaller re-factoring steps that are easier to review that'd be really great - eg. splitting out smaller common methods first.

Thanks !
Comment 3 Boris Egorov 2014-10-30 05:10:25 UTC
Sorry for the silence. I tried to create some unit test but stuck. What is exactly DrawDocShell? Where can I see it?
First patch on gerrit: https://gerrit.libreoffice.org/#/c/12148/
Comment 4 Commit Notification 2014-10-30 16:53:51 UTC
Boris Egorov committed a patch related to this issue.
It has been pushed to "master":

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

Related: fdo#84846 move code setting edit mode to a new method

It will be available in 4.4.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 Robinson Tryon (qubit) 2015-12-14 04:59:02 UTC Comment hidden (obsolete)
Comment 6 Commit Notification 2015-12-24 11:33:28 UTC
Fernando Pirani committed a patch related to this issue.
It has been pushed to "master":

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

tdf#84846 Merge identical GotoTreeBookmark into GetObjectIsMarked

It will be available in 5.2.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 Robinson Tryon (qubit) 2016-02-18 14:52:01 UTC Comment hidden (obsolete)
Comment 8 Tor Lillqvist 2016-03-08 06:03:31 UTC
aysemelikeyurtoglu, are you still working on this?
Comment 9 Tor Lillqvist 2016-03-08 06:05:39 UTC
If this bug is about only that specific source file, is this now actually done?
Comment 10 Michael Meeks 2016-03-08 08:34:30 UTC
Looks fixed to me; thanks ! =)