The text displayed in the first and second section of a combined fragment
widget are stored in the text variable, which was not set by the
ClassGeneralPage::apply method. This commit fixes this method's logic
BUG: 382279, 382282
Details
Details
- Reviewers
habacker - Group Reviewers
Umbrello - Commits
- R139:7f327bfaba35: Fix renaming of combined fragment sections' names
Re-play the steps listed in the tickets' descriptions
Diff Detail
Diff Detail
- Repository
- R139 Umbrello
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Comment Actions
looks good otherwise. After fixing the mentioned issues it could be applied to master because this patch changes the location of i18n calls which is not allowed in stable branches.
| umbrello/dialogs/pages/classgeneralpage.cpp | ||
|---|---|---|
| 373 | Please no java style. place '{' after a method name into the next line e.g. bool ClassGeneralPage::canApplyToUMLWidget() const | |
| 381 | see above | |
| 388 | see above | |
| 402 | see above | |
Comment Actions
Sure, that's actually my style too. I saw that if blocks are in Java style and I mistakenly thought that would apply to methods as well. I will fix it.
Comment Actions
I applied the comments regarding the braces.
Furthermore, I have fixed the patch, in that it was missing:
- re-naming for some variable to reflect their actual type and be less confusing in my opinion
- removes unused include directives
Comment Actions
While it is nice that you cover this, I do not like mixing the initial fix with unrelated changes, which makes it hard to see what has been really changed and what is only cosmetic. Think about the case you need to inspect single commits say in one year because there is an hidden issue. In case there are commits covering a single topic you can save much time by simply skipping the cosmetic and unrelated commits. With mixed commits you need to inspect every commit if it contains real changes or not.
I do not know if phabricator supports a set of patches in one request. If not feel free to open new review requests for each additional topic. You may collect removing unused include directives patches in a separate local git branch and post them as one patch later to a single review request.
Please make sure that you rebase your patch to git master as the branch may be updated in the meantime.