| Summary: | reduce copy/paste coding ... | ||
|---|---|---|---|
| Product: | LibreOffice | Reporter: | Michael Meeks <michael.meeks> |
| Component: | Impress | Assignee: | melikeyurtoglu <aysemelikeyurtoglu> |
| Status: | RESOLVED FIXED | ||
| Severity: | normal | CC: | mentoring, samuel.mehrbrodt |
| Priority: | medium | Keywords: | difficultyBeginner, easyHack, skillCpp, topicCleanup |
| Version: | 4.3.0.2 rc | ||
| Hardware: | Other | ||
| OS: | All | ||
| Whiteboard: | ToBeReviewed | ||
| Crash report or crash signature: | Regression By: | ||
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). 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 ! 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/ 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. Migrating Whiteboard tags to Keywords: (EasyHack DifficultyBeginner SkillCpp TopicCleanup ) [NinjaEdit] 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. JanI is default CC for Easy Hacks (Add Jan; remove LibreOffice Dev List from CC) [NinjaEdit] aysemelikeyurtoglu, are you still working on this? If this bug is about only that specific source file, is this now actually done? Looks fixed to me; thanks ! =) |
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 !