Command list improvs #357
Command list improvs #357
Conversation
|
I've merged #353. Would you please rebase? |
BackEndLib/Wchar.cpp
Outdated
| @@ -130,6 +131,13 @@ void AsciiToUnicode(const std::string &str, WSTRING &wstr) { | |||
| AsciiToUnicode(str.c_str(), wstr); | |||
| } | |||
|
|
|||
| //***************************************************************************** | |||
| const WSTRING AsciiToUnicode(const char *psz) { | |||
There was a problem hiding this comment.
Returning by const value doesn't really help here, assuming WSTRING is the same as this one.
| const WSTRING AsciiToUnicode(const char *psz) { | |
| WSTRING AsciiToUnicode(const char *psz) { |
I would also look into [[nodiscard]] for function signatures like these. It is a C++17 feature but there are equivalent compiler-specific attributes since C++98. It produces a warning when the return value of AsciiToUnicode is discarded, avoidining mistakenly assuming that psz gets mutated rather than a new value being returned:
| const WSTRING AsciiToUnicode(const char *psz) { | |
| [[nodiscard]] WSTRING AsciiToUnicode(const char *psz) { |
There was a problem hiding this comment.
The [[nodiscard]] suggestion applies to pretty much every function here, or every function returning a value that should never be ignored.
BackEndLib/Wchar.cpp
Outdated
| for (WSTRING::const_iterator it = source.begin(); it != source.end(); ++it) | ||
| lowercased += towlower(*it); | ||
|
|
||
| return lowercased; |
There was a problem hiding this comment.
C++11 range-based for is cleaner here:
| for (WSTRING::const_iterator it = source.begin(); it != source.end(); ++it) | |
| lowercased += towlower(*it); | |
| return lowercased; | |
| for (char c : source) | |
| lowercased += towlower(c); | |
| return lowercased; |
BackEndLib/Wchar.cpp
Outdated
| } | ||
|
|
||
| //***************************************************************************** | ||
| const std::vector<WSTRING> WCSExplode(WSTRING const &source, WCHAR const delimiter) |
There was a problem hiding this comment.
Ditto, returning by const value is not useful and inhibits move semantics. See https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#f20-for-out-output-values-prefer-return-values-to-output-parameters
BackEndLib/Wchar.cpp
Outdated
| for (std::vector<WSTRING>::const_iterator iter = needles.begin(); iter < needles.end(); ++iter) | ||
| { | ||
| if (haystack.find(*iter) == WSTRING::npos) | ||
| return false; | ||
| } | ||
|
|
||
| return true; |
There was a problem hiding this comment.
From <algorithm>, C++11:
return std::all_of(needles.begin(), needles.end(),
[&haystack]
{
return haystack.find(*iter) != WSTRING::npos;
});Consider std::none_of as well, might be more readable due to the avoided negation in the body of the lambda expression.
DROD/CharacterDialogWidget.cpp
Outdated
| const UINT CCharacterDialogWidget::INDENT_PREFIX_SIZE = 6; | ||
| const UINT CCharacterDialogWidget::INDENT_TAB_SIZE = 3; | ||
| const UINT CCharacterDialogWidget::INDENT_IF_CONDITION_SIZE = 5; |
There was a problem hiding this comment.
If these are purely hardcoded constants, consider using a constexpr variable (C++11), or an inline constexpr variable if C++17 support is available. Just makes it more obvious and has a stronger guarantee that the value is purely known at compile-time.
|
@SuperV1234 I am afraid we're currently not targeting C++11 :(. |
What platform is stopping you? Seems a bit of a weird decision in 2020. |
Our current OS X development setup. |
…f blocks + displaying a lot of commands now works faster because indenting now happens once instead once for each command
EvidentlyCube
force-pushed
the
command-list-improvs
branch
from
d3f24b3
to
c72b841
Compare
|
@mrimer PR updated, I also fixed labels not having negative indent. |
| @@ -189,6 +190,9 @@ const UINT LIST_LINE_HEIGHT = 22; | |||
| const SURFACECOLOR PaleRed = {255, 192, 192}; | |||
|
|
|||
| std::map<UINT, UINT> CCharacterDialogWidget::speechLengthCache; | |||
| const UINT CCharacterDialogWidget::INDENT_PREFIX_SIZE = 8; | |||
There was a problem hiding this comment.
These constants should be consolidated with the analogous magic numbers featured elsewhere in the code, e.g., CCommandListBoxWidget.
There was a problem hiding this comment.
@mrimer I'm not quite sure what needs to be consolidated where. These constants appear to be grouped with the other ones in this file, and they don't appear to share a purpose with the ones in CCommandListBoxWidget. That file does have some constants littered about, but they are in the place where they're being used, so it doesn't seem like much of an issue.
There was a problem hiding this comment.
I appreciate you looking into this. I possibly misinterpreted the values. IIRC, to me, it looked like there are identical dimension values in both places, e.g., 8 pixels for an indent size, that possibly need to remain in lockstep in both places for things to work correctly. If this isn't the case, I'm fine with leaving things as-is for now.
|
@mrimer In light of skell's departure from DROD development, could this be merged as-is? The feature is complete, and I can whip up a PR to move the magic number to live with its friends. |
|
Merging. My last request can be applied as a follow-on PR. |
This depends on changes from #353.
Relevant thread