Improvement: Improved Person View to Include a Larger Portrait & Upgraded Several Components; Moved Medical Record Button to Personnel Right-Click Menu#6894
Conversation
…aded Several Components; Moved Medical Record Button to Personnel Right-Click Menu
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6894 +/- ##
============================================
- Coverage 11.69% 11.68% -0.01%
+ Complexity 6657 6651 -6
============================================
Files 1100 1100
Lines 141324 141324
Branches 21848 21885 +37
============================================
- Hits 16526 16515 -11
- Misses 123098 123112 +14
+ Partials 1700 1697 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Removed 'For New Dev Cycle' tag. Updated to be suitable for 50.06 |
|
I am approving, but I dont know what the green and red arrows are supposed to mean |
|
They mean that the skill is being buffed or debuffed by something. The tooltip explains what the actual modifier is. It's largely irrelevant (and will be mostly unseen) in 50.06, but will be very important in 50.07 when attribute score modify every skill |
Scoppio
left a comment
There was a problem hiding this comment.
approved, but there are things to consider
| tooltip.append(getFormattedTextAt(RESOURCE_BUNDLE, | ||
| "tooltip.format.spa", | ||
| (spaModifier > 0 ? "+" : "") + spaModifier)); |
There was a problem hiding this comment.
the symbols should also be in the i18n, Iknow this is fringe, but I always remember the problem with quotation marks, latin script uses "those", and they are usually "open - close", french uses a special double chevron << for important stuff >>, other languages use simple quotation mark, etc.
There was a problem hiding this comment.
That is very tedious but I'll see it done.
| * @author Jay Lawson (jaylawson39 at yahoo.com) | ||
| */ | ||
| public class SkillType { | ||
| private static final String RESOURCE_BUNDLE = "mekhq.resources." + SkillType.class.getSimpleName(); |
There was a problem hiding this comment.
same issue as before, this adds a level of indirection that is not necessary.
There was a problem hiding this comment.
Resolved in upcoming commit
| public String getFlavorText(boolean includeHtmlTags, boolean includeAttributes) { | ||
| String key = getResourceBundleKey(); | ||
| String rawFlavorText = getTextAt(RESOURCE_BUNDLE, key + ".flavorText"); | ||
|
|
||
| String htmlOpenTag = includeHtmlTags ? "<html>" : ""; | ||
| String htmlCloseTag = includeHtmlTags ? "</html>" : ""; | ||
|
|
||
| if (!includeAttributes) { | ||
| return htmlOpenTag + rawFlavorText + htmlCloseTag; | ||
| } | ||
|
|
||
| String flavorText = htmlOpenTag + rawFlavorText + "<br>(" + firstAttribute.getLabel(); | ||
|
|
||
| if (secondAttribute != NONE) { | ||
| flavorText += ", " + secondAttribute.getLabel() + ')' + htmlCloseTag; | ||
| } | ||
|
|
||
| return flavorText; | ||
| } |
There was a problem hiding this comment.
There are better ways to do this... I think we should try to find some way to do this that can be easily replicated around the project bc its very commonly a problem we have to deal with. Dont worry about this piece of code atm, its fine.
| PersonnelTabView.GENERAL : | ||
| choicePersonView.getSelectedItem(); | ||
| final XTableColumnModel columnModel = (XTableColumnModel) getPersonnelTable().getColumnModel(); | ||
| getPersonnelTable().setRowHeight(UIUtil.scaleForGUI(15)); |
There was a problem hiding this comment.
I suggest against doing static imports too liberally, as it can very quickly start to hide complexities and dependencies between modules
There was a problem hiding this comment.
I see your point, I hadn't considered that
| if (person.getAwardController().hasAwards()) { | ||
| if (person.getAwardController().hasAwardsWithRibbons()) { | ||
| PersonAwardController awardController = person.getAwardController(); | ||
| if (awardController.hasAwards()) { |
There was a problem hiding this comment.
I know it is better now, but every "big component" should be a function call, so it is easier to find and fix/change things.
There was a problem hiding this comment.
It's better, but I still don't like it. There is a lot of boilerplate. Lemme have a think. I know I can handle this whole method better, just need to figure out how...
There was a problem hiding this comment.
I decided to just go ahead and split it out. I still feel there is a better way to handle this whole method, but I'm at a point where I can't see it and moving it out resolves the problem without me inventing new issues.
|
| tooltip.append(getFormattedTextAt(RESOURCE_BUNDLE, | ||
| "tooltip.format.linkedAttribute", | ||
| firstLinkedAttribute.getLabel(), | ||
| (firstLinkedAttributeModifier > 0 ? additionSymbol : "") + firstLinkedAttributeModifier)); |
Check warning
Code scanning / CodeQL
Useless comparison test Warning
| tooltip.append(getFormattedTextAt(RESOURCE_BUNDLE, | ||
| "tooltip.format.linkedAttribute", | ||
| secondLinkedAttribute.getLabel(), | ||
| (secondLinkedAttributeModifier > 0 ? additionSymbol : "") + secondLinkedAttributeModifier)); |
Check warning
Code scanning / CodeQL
Useless comparison test Warning
Requires #6839
Fix #4451
Fix #6326
Dev Notes
The goal of this change was to break up the visual 'blocking' of the display so that users could more easily pick out the information they needed, while also providing better support for attribute modifiers and role-play skills.
The little green 'medical' symbol previously linked the user to the characters' medical history. That has been removed, instead the medical report can be accessed from the characters' right-click menu.
Before
After
(The genealogy is further down in the view)