Skip to content
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

Add functionality to Time Schedule Editor #8066

Merged
merged 26 commits into from
Dec 14, 2023

Conversation

babaissarkar
Copy link
Contributor

@babaissarkar babaissarkar commented Nov 24, 2023

Changes :

  1. Pressing the OK button in the Time Schedule Editor in the Map Editor now modifies the current schedule.
  2. Save Scenario As menu item now writes the current assigned schedule to utils/schedule.cfg.
  3. color_slider_callback() has new parameter to identify slider. Previously with one callback, the g and b sliders in the ui were not updating their values.
  4. New Description textbox added in custom_tod dialog that allows to view and modify the description key of the [time] tag.
  5. The Time Schedule button/hotkey now disabled unless the user is editing a scenario.
  6. Preview button for color sliders. Map color changes can be previewed without closing and reopening the dialog.
  7. WML translatable attributes written with leading underscore '_'.

@github-actions github-actions bot added UI User interface issues, including both back-end and front-end issues. Editor Map/scenario editor issues. labels Nov 24, 2023
@stevecotton
Copy link
Contributor

Note about translatable strings - this reuses an already-translatable string, Description: there's no new ones.
👍

@Pentarctagon
Copy link
Member

Out of curiosity, does this write out the translatable attributes with the leading underscore?

@doofus-01
Copy link
Member

The Time Schedule button/hotkey now disabled unless the user is editing a scenario.

Just wondering, would it be difficult to have the schedule button either pull up existing schedules or temporarily show whatever tint was chosen in the dialog, but not save anything? It would be convenient for testing maps at various times of day, either for the map design or terrain graphics drawing.

I could swear it used to load predefined schedules, but I'm probably misremembering.

@babaissarkar
Copy link
Contributor Author

babaissarkar commented Nov 25, 2023

Out of curiosity, does this write out the translatable attributes with the leading underscore?

Probably not yet. I'll see if I can fix it.
Edit : Problem is deep inside how config_writer writes the config object.

@babaissarkar
Copy link
Contributor Author

The Time Schedule button/hotkey now disabled unless the user is editing a scenario.

Just wondering, would it be difficult to have the schedule button either pull up existing schedules or temporarily show whatever tint was chosen in the dialog, but not save anything? It would be convenient for testing maps at various times of day, either for the map design or terrain graphics drawing.

I could swear it used to load predefined schedules, but I'm probably misremembering.

Adding preview for the tints should be easy. I'm not so sure about schedule, but I'll look into it.

@babaissarkar
Copy link
Contributor Author

babaissarkar commented Nov 25, 2023

I could swear it used to load predefined schedules, but I'm probably misremembering.

@doofus-01 It did, if you choose a schedule in the Assign Time Schedule item after clicking the Time button.

Allows to quickly preview changes in color without closing dialog.
@CelticMinstrel
Copy link
Member

Where's the code that writes the addon's _main.cfg? Does it write a [textdomain] tag?

@babaissarkar
Copy link
Contributor Author

babaissarkar commented Nov 27, 2023

Where's the code that writes the addon's _main.cfg? Does it write a [textdomain] tag?

@CelticMinstrel The code that writes the _main.cfg is in editor_main.cpp. The line you're looking for is
*stream << "#textdomain wesnoth-" << addon_id << "\n"
The textdomain is same as the one used by me.

Also, this part already existed before my changes.

@CelticMinstrel
Copy link
Member

I see it. Technically the lines I was looking for were the ones just after that line, but either way it looks like the [textdomain] is written so all is fine.

@Pentarctagon
Copy link
Member

@babaissarkar we're currently in the string freeze leading up to the 1.18.0 release, so either:

  • The two translatable strings need to be remove/replaced with something that's already existing elsewhere
  • This PR will need to wait until after 1.18.0 is released.

@babaissarkar
Copy link
Contributor Author

@Pentarctagon New Strings replaced with existing ones, see fcd7e80.

@Pentarctagon
Copy link
Member

Any other issues that need to be addressed before merging?

@babaissarkar
Copy link
Contributor Author

Any other issues that need to be addressed before merging?

@Pentarctagon No, there shouldn't be anything else.

@Pentarctagon
Copy link
Member

An entry should be added to https://github.com/wesnoth/wesnoth/tree/master/changelog_entries

Other than that, I'll merge this in a couple days if there are no other comments.

@CelticMinstrel
Copy link
Member

CelticMinstrel commented Dec 11, 2023

The changelog file should probably be named differently. If unsure you could use the PR number.

@babaissarkar
Copy link
Contributor Author

babaissarkar commented Dec 11, 2023

@CelticMinstrel File renamed to 8066_time_schedule_editor.md

Schedule name is no longer generated. Schedule ID is used in Assign Schedule menu
if schedule name is empty.
@babaissarkar
Copy link
Contributor Author

babaissarkar commented Dec 13, 2023

I've implemented changes suggested by @soliton- and @CelticMinstrel in fb6d8c9.
In particular :

  1. OK button is disabled unless user enters a name for the custom schedule. Name is no longer generated.
  2. In case the name attribute is empty, the id of the schedule is used in the Assign Schedule menu.
  3. Autogenerated Schedule ID now prefixed with the addon ID to avoid clashes among addons.

@Pentarctagon
Copy link
Member

Alright, I'll be merging this tomorrow then if there are no additional comments.

Autogenerated ID is only used as a fallback.
@Pentarctagon Pentarctagon merged commit f1f67ae into wesnoth:master Dec 14, 2023
18 checks passed
@Pentarctagon
Copy link
Member

Thanks for sticking with this!

babaissarkar added a commit to babaissarkar/wesnoth that referenced this pull request May 22, 2024
also add tooltip to preview button
and update copyright notice (missing from wesnoth#8066)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Building Build-time issues. Editor Map/scenario editor issues. macOS OS-specific issues that apply to Apple macOS. Schema UI User interface issues, including both back-end and front-end issues.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants