Skip to content

Refactor MiniEditor enabled state to use Preferences API - #5853

Open
zoldyck13 wants to merge 2 commits into
aseprite:mainfrom
zoldyck13:fix-config-api
Open

zoldyck13 wants to merge 2 commits into
aseprite:mainfrom
zoldyck13:fix-config-api

Conversation

@zoldyck13

Copy link
Copy Markdown

=### Description
This pull request migrates the "MiniEditor" configuration from the legacy ini_file system to the modern app::Preferences API.

  • Updated pref.xml to include the miniEditor section.
  • Refactored src/app/ui/preview_editor.cpp to use the Preferences API for the enabled state.
  • Cleaned up redundant legacy config calls, improving configuration consistency and aligning the feature with current codebase standards.

Contribution Agreement

I declare that my contributions are not co-authored using a generative AI technology.

I agree that my contributions are licensed under the Individual Contributor License Agreement V4.0 ("CLA") as stated in https://github.com/igarastudio/cla/blob/main/cla.md

I have signed the CLA following the steps given in https://github.com/igarastudio/cla#signing

@zoldyck13
zoldyck13 requested a review from dacap as a code owner June 5, 2026 16:21
@aseprite-bot

Copy link
Copy Markdown
Collaborator

clang-tidy review says "All clean, LGTM! 👍"

@dacap dacap left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your PR @zoldyck13, actually if we're going to refactor the [MiniEditor] to Preferences, it'd be nice to use the "preview" section and start listening to the changes of the preview.enabled option so we sync the "Preview" window visibility with the option value.

Comment thread data/pref.xml
<option id="allow_nonlinear_history" type="bool" default="false" />
<option id="show_tooltip" type="bool" default="true" />
</section>
<section id="miniEditor" text="Mini Editor">

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're using another kind of standard for these preferences, it should be:

    <section id="mini_editor" text="Mini Editor">

Anyway we should move this preference to the already existing "preview" section. E.g.

    <section id="preview" text="Preview">
      <option id="enabled" type="bool" default="true" />
      <option id="play_once" type="bool" default="false" />
      <option id="play_all" type="bool" default="false" />
      <option id="play_subtags" type="bool" default="true" />
    </section>

setWantFocus(false);

m_isEnabled = get_config_bool("MiniEditor", "Enabled", true);
m_isEnabled = app::Preferences::instance().miniEditor.enabled();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually if we refactor this, we should impl this "Preview" visibility switching listening the app::Preferences::instance().preview.enabled() changes, and if one script changes the value the Preview should change its visibility too.

{
set_config_bool("MiniEditor", "Enabled", m_isEnabled);
app::Preferences::instance().miniEditor.enabled(m_isEnabled);
save_window_pos(this, "MiniEditor");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

save_window_pos() is called from kCloseMessage, is this call needed? What case does it fix?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants