Some fixes from me #68
Conversation
See also: Serg-Norseman#59 Signed-off-by: ‡Ruslan Garipov <garipov.rn@yandex.ru>
See also: Serg-Norseman#61 Signed-off-by: ‡Ruslan Garipov <garipov.rn@yandex.ru>
When user deleted the last entry in an object list in a database window, the right (hyper view) panel was continuing to show information about the removed object. This commit fixes it. See also: Serg-Norseman#61. Signed-off-by: ‡Ruslan Garipov <garipov.rn@yandex.ru>
This commit fixes text of the note removing confirmation. Confirmation text template (`LSID.LSID_NoteDeleteQuery`) contains a format placeholder, but GK didn't use it. Signed-off-by: ‡Ruslan Garipov <garipov.rn@yandex.ru>
This commit ignore "empty" notes on creation. This commit also fix some ENU localization. See also: Serg-Norseman#58 Signed-off-by: ‡Ruslan Garipov <garipov.rn@yandex.ru>
Current coverage is 37.71% (diff: 11.02%)@@ master #68 diff @@
==========================================
Files 322 322
Lines 50087 50193 +106
Methods 4585 4587 +2
Messages 0 0
Branches 4740 4746 +6
==========================================
- Hits 18950 18931 -19
- Misses 30516 30641 +125
Partials 621 621
|
| /* 241 */ "Are you sure you want to detach link to note?", | ||
| /* 242 */ "Are you sure you want to detach link to multimedia?", | ||
| /* 243 */ "Are you sure you want to detach link to source?", | ||
| /* 244 */ "Are you sure you want to remove the association?", |
There was a problem hiding this comment.
To make changes to the default localization only once, you need to make changes only to LangMan. And then perform one run with the activated line SaveDefaultLanguage() in the constructor of the class MainWin. Then deactivate the line.
There was a problem hiding this comment.
So... There's nothing I have to change in this PR because of this review entry? 👀
| view = mLocationSummary; | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Two requests: to emphasize a separate "case" - use empty strings. If two or more "case" have a common implementation, an empty string is not needed. And second, if the body of the "case" consists of one to five lines and there are no variables that overlap in scope with other "case" - not to do a block "{ }".
There was a problem hiding this comment.
Would I need to fill the document with the conventions...
There was a problem hiding this comment.
Would I need to fill the document with the conventions
No, this ain't necessary.
Are you talking about automatic generating of 'locales/english.sample' file from langman.cs one? 'locales/english.sample' file is stored within this repo; I have to fix it at the same time with langman.cs. |
| msg = string.Format(LangMan.LS( | ||
| LSID.LSID_PersonDeleteQuery), | ||
| ((GEDCOMIndividualRecord)record).GetNameString( | ||
| true, false)); |
There was a problem hiding this comment.
For me so uncomfortable. Let's at least that such intermediate format choose:
msg = string.Format(LangMan.LS(LSID.LSID_PersonDeleteQuery),
((GEDCOMIndividualRecord)record).GetNameString(true, false));
Hereinafter.
There was a problem hiding this comment.
As you wish. But you definitely exceed 80-characters per line limit.
I revert the code format back to your initial style -- one LONG expression per one line. I hate that, just like I hate this:
msg = string.Format(LangMan.LS(LSID.LSID_PersonDeleteQuery),
((GEDCOMIndividualRecord)record).GetNameString(true, false));Hereinafter 😆
There was a problem hiding this comment.
I'm sorry. I just don't understand, in terms of readability, when at least the first and relatively short argument divorced from the call.
It is possible and on another to write:
msg = string.Format(
LangMan.LS(LSID.LSID_PersonDeleteQuery),
((GEDCOMIndividualRecord)record).GetNameString(true, false));
What here bad?
There was a problem hiding this comment.
Nothing. Except the fact that it still exceeds 80 characters limit 😃
I will try to accept your code conventions, I will! 
There was a problem hiding this comment.
You shouldn't 😆 You set the rules as owner of this repo, and that's OK! And I have to obey those rules! 👍
| { | ||
| value = string.Format("#{0}", | ||
| record.GetId().ToString()); | ||
| } |
There was a problem hiding this comment.
It is better to make a separate function (in GKUtils) - it can be embedded in one or two places.
Signed-off-by: Ruslan Garipov <brigadir15@gmail.com>
|
@Serg-Norseman I fixed all your comments! |
This PR fixes some issues.
ChangeLog:
2016-09-27 Ruslan Garipov brigadir15@gmail.com
(GKUI::BaseWin::GetHyperViewByType): Add new method.
(GKUI::BaseWin::RecordNotify): Clear hyper view if necessary.
(GKUI::BaseWin::RecordDelete): Reformat code. Change "Remove note" confirmation text building.