-
-
Notifications
You must be signed in to change notification settings - Fork 471
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Display music metadata in the level editor #2674
base: master
Are you sure you want to change the base?
Conversation
a254832
to
f595359
Compare
f595359
to
32ff1d7
Compare
32ff1d7
to
2bc9f35
Compare
src/editor/object_option.cpp
Outdated
@@ -440,7 +442,7 @@ FileObjectOption::to_string() const | |||
void | |||
FileObjectOption::add_to_menu(Menu& menu) const | |||
{ | |||
menu.add_file(get_text(), m_value_pointer, m_filter, m_basedir, m_path_relative_to_basedir); | |||
menu.add_file(get_text(), m_value_pointer, m_filter, m_basedir, m_path_relative_to_basedir, -1, m_generate_help_text_for_file); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
m_generate_help_text_for_file
somehow suggests that this is boolean. No idea if I can think of another name, though.
src/editor/object_settings.cpp
Outdated
|
||
std::string help_text = | ||
title_or_filename + (author.empty() ? "" : "\n by " + author) | ||
+ (license.empty() ? "" : "\nLicense: " + license); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fmt::format
should be used here to support translations.
fmt::format(fmt::runtime(_("{} by {}")), title_or_filename, author)
in one variable, if an author is provided. If not, only copy the title to this variable (by the way, if no info is provided, maybe nothing should be shown at all?).
"\n" + fmt::format(fmt::runtime(_("License: {}")), license)
in another variable, if license is provided. If not, keep empty.
Return the two variables concatenated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Vankata453, good catch: I updated the code to support translations. However, while I wrote the code, I discovered that there's an issue preventing "{}" substitutions from working in the current Git master version. I reported this issue in issue #2700.
I am currently working on code changes based on your recommendation. There are some differences I would lke to make to your suggestions. Notably, I would like to translate just the word License with _("License") + ": {}"
to take advantage of an existing .po file msgid "License" translations.
There may be an issue with _("{} by {}") msgid for some languages like Japanese, where it seems that the order of the first two arguments should be swapped, relative to the English message. For example, '"Piano Man" by Billy Joel' is Birī Joeru no `piano man' in Japanese (according to Google Translate). I'll lookup how to support argument swapping with tinygettext.
Currently, there is a newline separating the TITLE and the "by AUTHOR" parts. Do you know if _("{}\nby {}") works?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Notably, I would like to translate just the word License with
_("License") + ": {}"
to take advantage of an existing .po file msgid "License" translations.
Seems fine.
Currently, there is a newline separating the TITLE and the "by AUTHOR" parts. Do you know if _("{}\nby {}") works?
I believe it should.
Thanks for all your feedback @tobbi and @Vankata453! I pushed commits that fix all the easy code style changes, I will work on resolving your remaining feedback tomorrow. |
i have enabled workflows in this pr |
What is the current state of this PR? |
08ca4ec
to
60276d2
Compare
@mrkubax10 I am resolving remaining feedback on this PR. In particular, right now I am working on supporting translations. |
39b5cef
to
b85fb20
Compare
Looking good! My only style complaint would be that |
May I suggest something like |
fafa7f2
to
a0db7f7
Compare
What still needs to get done here? |
To Do
This feature was functionally complete, but refactoring it to use the Vankata's item_processor parameter (instead of the separate specific generate_help_text_for_file) and rebasing it on the latest master is still a work in progress. Also, there are English phrases within the author field data that should be translated (some author values include a lot of info, including roles different contributors had for a given song). Finally, there are a few new music files added to SuperTux since December that need the metadata fields added to their .music files. Current Progress on RefactoringLast week I worked on the refactoring, and I am planning on continuing the refactor this week. UPDATE March 4: I fixed the illegal instruction exception by fixing return statements in the item_processor function. Now I am continuing to debug other issues until the refactored code works. |
82cca71
to
b919e6b
Compare
d776cbe
to
5fd0d6d
Compare
Is this done? |
This is work in progress. All functionality and translations are complete. Still left to do are updating music metadata, like title, author and license, for recent music added to the game, and refactor to extract music help text item processor function out of object_settings.cpp. I'm planning on resuming my work for this PR this week. |
Thank you! 👍 |
abecc37
to
4969e2f
Compare
8f50cda
to
d613c1f
Compare
…erTux#2617) Whenever any of author, title and license fields are defined for music files, help text showing this music metadata is displayed when hovering over ".music" files in the level editor music object file browser. This commit adds both the software implementation and author, title, and license values added to SuperTux music files in data/music. The game credits and data/AUTHORS are updated in some cases where there is new authorship data. Fixes SuperTux#2617.
d613c1f
to
c579652
Compare
This pull request is ready for code review and merging! I completed all the remaining work that I mentioned in my last status update 3 weeks ago. The code now uses |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works great! You can see all important information about a music file:
Left 2 minor code-related comments. If you agree with any of them, I am also up to make the changes listed (if you want me to, of course). Offering that, so you don't have to do any code-related work on this no more if you don't want to, seeing the effort you put in throughout all this time, encouraged by all the previous comments.
Co-authored-by: Vankata453 <78196474+Vankata453@users.noreply.github.com>
Resolves code review issue pointing out that this standalone function is out of place among the classes of the src/gui folder. item_processor is now specified using an anonymous function in the method ObjectSettings::add_music().
Thanks for the feedback: I fixed both of them! I enabled "Allow edits by maintainers" for this PR so you are welcome to add commits to the PR repo if you prefer. However, I am happy to fix any additional feedback you mention here on GitHub. |
[ci skip]
[ci skip]
[ci skip]
Fixes #2617.
When navigating or hovering over menu items in the level editor music object settings file browser, information about a song's author, license, and title will now be displayed in the menu item help text at the bottom of the screen.
This pull request adds:
The author, license and title fields are option for music files. SuperTux simply omits the display of any fields that are not present in music files, and if none of these fields are defined for a music file (like christmas_theme.music, currently), then help text does not display for that song.