♻️ (MagicCard): Add language byte to MagicCard #1048
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
File comparision analysis report🔖 Info
Click to show memory sections📝 SummaryClick to show summary
🗺️ Map files diff outputClick to show diff list
|
File comparision analysis report🔖 Info
Click to show memory sections📝 SummaryClick to show summary
🗺️ Map files diff outputClick to show diff list
|
Codecov Report
@@ Coverage Diff @@
## develop #1048 +/- ##
========================================
Coverage 95.99% 95.99%
========================================
Files 133 133
Lines 3171 3173 +2
========================================
+ Hits 3044 3046 +2
Misses 127 127
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
ladislas
approved these changes
Oct 3, 2022
libs/RFIDKit/include/MagicCard.h
Outdated
| @@ -30,6 +30,8 @@ struct MagicCard { | |||
| return both; | |||
| } | |||
|
|
|||
| [[nodiscard]] constexpr auto getLanguage() const -> uint8_t { return _tag.data.at(language_byte_index); } | |||
There was a problem hiding this comment.
not asking for a change, but after re-reading the whole code, having at getXXX feels a bit verbose. It's just a simple struct, card.id() or card.language() would be good enough.
to keep in mind for a future refactor ;)
There was a problem hiding this comment.
To make it more readable and using Language inside MagicCard's architecture, use uint16_t _id & Language _language as variables instead of rfid::Tag _tag.
libs/RFIDKit/include/MagicCard.h
Outdated
| @@ -30,6 +30,8 @@ struct MagicCard { | |||
| return both; | |||
| } | |||
|
|
|||
| [[nodiscard]] constexpr auto getLanguage() const -> uint8_t { return _tag.data.at(language_byte_index); } | |||
There was a problem hiding this comment.
Instead of returning a byte (0x01, 0x02, ...) I think it is better to wrap it into an Enum to understand what is happening on use
enum class TagLanguage{
None = 00,
fr_FR = 01,
en_US = 02
} if (card.getLanguage() == TagLanguage::fr_FR) {
_videokit.displayImage("/fs/home/img/system/robot-misc-choose_activity-fr_FR.jpg");
} else if (card.getLanguage() == TagLanguage::en_US){
_videokit.displayImage("/fs/home/img/system/robot-misc-choose_activity-en_US.jpg");
}There was a problem hiding this comment.
Going further, we can have a python dictionary like or struct as Color.h in ActivityKit to get language tag from the byte in order to directly set the right tag in the path
_videokit.displayImage({image_system_folder} + {id_converted_name} + '-' + {tag_language} + ".jpg");
We can abord this when we will simplify use of videokit by dropping the folder and extension, {image_system_folder} and ".jpg" in the above case.
There was a problem hiding this comment.
I agree with @YannLocatelli
Just a small note: I would call the enum just Language and keep in struct MagicCard to access/use it via MagicCard::Language
HPezz
force-pushed
the
hugo/feature/Add-language-byte-in-MagicCard
branch
from
e9fda6c
to
89d51ff
Compare
HPezz
force-pushed
the
hugo/feature/Add-language-byte-in-MagicCard
branch
from
89d51ff
to
4097645
Compare
YannLocatelli
approved these changes
Oct 4, 2022
There was a problem hiding this comment.
That's look pretty good!
Except applying suggestion of @ladislas by setting Language in MagicCard struct, everything is good to me 👌
ladislas
requested changes
Oct 10, 2022
libs/RFIDKit/include/MagicCard.h
Outdated
Comment on lines
14
to
18
| enum class Language | ||
| { | ||
| fr_FR = 1, | ||
| en_US = 2 | ||
| }; |
There was a problem hiding this comment.
Can you move this inside struct MagicCard?
HPezz
force-pushed
the
hugo/feature/Add-language-byte-in-MagicCard
branch
from
4097645
to
46e154b
Compare
HPezz
force-pushed
the
hugo/feature/Add-language-byte-in-MagicCard
branch
from
46e154b
to
1a77f51
Compare
|
Kudos, SonarCloud Quality Gate passed! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.