Add symbols editor #761

Frostbyte0x70 · 2024-06-19T23:28:48Z

Adds a new screen that can be used to view and edit the value of data symbols defined in pmdsky-debug. Requires SkyTemple/skytemple-files#505.

The new screen supports editing values as text. Symbols of known enum types use dropdowns or autocomplete textboxes, so the user can check the list of valid values. Boolean values use checkboxes. If the user inputs an invalid or out of range value, an error message is displayed.

Now that this editor exists, I see little purpose for the view located under Patches > Symbols. Maybe we should consider removing / hiding it.

I also think that Lists > Misc. Settings is outdated now, and I wouldn't mind removing it, mainly because:

All the values listed there can already be edited in this new screen, which isn't much harder to navigate
The Misc. Settings view contains arbitrary fields
Any future changes to those symbols in pmdsky-debug will not be reflected on the Misc. Settings view unless manually updated
The input for belly lost per turn cannot handle negative values, unlike the one in the new symbols view
But I don't have a strong opinion on the matter, it's just a suggestion.

Frostbyte0x70 · 2024-06-29T11:01:45Z

skytemple/module/symbols/model_getter.py:133:18: error: Missing positional argument "types" in call to "new" of "ListStore" [call-arg]
skytemple/module/symbols/model_getter.py:133:36: error: Argument 1 to "new" of "ListStore" has incompatible type "list[type]"; expected "int" [arg-type]

As stated in the code, the type hint Gtk.ListStore.new seems to be incorrect. It says the function takes two parameters, but https://lazka.github.io/pgi-docs/Gtk-3.0/classes/ListStore.html#Gtk.ListStore.new says it takes one. How do I suppress this?

skytemple/module/symbols/model_getter.py:143:57: error: Argument 1 to "Gender" has incompatible type "u8"; expected "str" [arg-type]

I copied this from existing code. Not sure what to do here.

Error: skytemple/module/symbols/widget/main.py:89:38: W605 Invalid escape sequence: $
Error: skytemple/module/symbols/widget/main.py:89:45: W605 Invalid escape sequence: \d
Error: skytemple/module/symbols/widget/main.py:89:49: W605 Invalid escape sequence: $

???
The regex is valid. Do I just suppress this?

theCapypara · 2024-06-29T12:10:23Z

ListStore.new should not be used, use the normal constructor instead:

skytemple/skytemple/module/lists/widget/iq.py

Line 296 in 2c469f5

store: Gtk.ListStore = Gtk.ListStore(*[int, str] + [str] * num_types)
That is a quirk of our custom Enums with metadata that mypy doesn't understand, you add a # type: ignore in that line
Use raw strings for regexs: r"$[$#](\d+)$$". If you don't, then Python will interpret the backslashes and they won't be in the regex.

Frostbyte0x70 · 2024-07-08T15:17:15Z

@theCapypara Not sure if you saw it, but this is ready for review now. Just wanted to make sure you didn't miss it.

theCapypara

Great work!! I have some remarks, but nothing big.

skytemple/data/widget/symbols/main.ui

theCapypara · 2024-07-12T12:47:59Z

skytemple/data/widget/symbols/main.ui

+        <property name="shadow-type">in</property>
+        <property name="propagate-natural-height">True</property>
+        <child>
+          <object class="GtkTreeView" id="symbols_treeview">


This tree view requires horizontal scrolling in a lot of cases which is not great. However we can fix this with the GTK 4 migration. We really need to care more about smaller screen sizes.

To make it a bit more bearable however, please switch the value and description column around. If you can figure out if there is a good way to make it so that only a small portion of description is shown by default and there is a way to show more details that would be even better.

This tree view requires horizontal scrolling in a lot of cases

Does it? I've never had to scroll horizontally. My screen resolution is on the high end (1080p), but there's always some space available on the right side.

The easiest fix would be to reduce the width limit for the description column, but I want to make sure it's necessary first. It might look worse in bigger screens if we do.

skytemple/module/symbols/module.py

skytemple/module/symbols/widget/main.py

theCapypara · 2024-07-12T12:54:16Z

skytemple/module/symbols/widget/main.py

+            new_value_final = str(int(_id))
+            self.on_value_changed(path, new_value_final)
+
+    def on_value_changed(self, path: str, new_value: str, model_iter: Optional[Gtk.TreeIter] = None):


Please add visual feedback when any row is changed. There should be an asterisks for unsaved changes similiar to how it is done in the main item tree. Maybe make changed rows bold as well.

Where exactly should the asterisk appear? I don't think I can access the individual cells to change the text.
Bolding should be doable though. Do I just bold the entire row?

Sure.
As for the asterisks, there are a few options, potentially the eaiest is to add a new cell renderer to the first column that renders a column in the store that either contains "" or "*", this is similiar to how it works in the main item store.

Uh... how do I know when the ROM has been saved so I can restore the normal text display?

Ah, right. I don't remember if there is a hook for that, maybe not. But tbh I think it's also fine if you don't reset it while the symbol editor is still open for now.
I guess another issue is also that you don't know what changed when you re-open. But that is also fine.
I guess this way it's more like a "this is what you changed since you last opened the symbol editor". In that case just using bold is probably better, no asterisk, since the asterisk is assosiated with the ROM itself not being saved, that might confuse people.

Alright then, just bold for now.

skytemple/module/symbols/module.py

theCapypara · 2024-07-12T14:36:21Z

skytemple/module/symbols/model_getter.py

+from skytemple_files.hardcoded.symbols.manual.enums import get_enum_values, get_all_enum_types
+from skytemple_files.hardcoded.symbols.unsupported_type_error import UnsupportedTypeError
+
+_instance: "ModelGetter | None" = None


Please put from __future__ import annotations at the top of all source files and then use regular type annotations (not strings)

Ohhhh, so that's how you avoid ImportErrors when referencing a type before or during its declaration. The only workaround I knew was using a string to declare the type. Good to know.

I've changed it in all files that needed it. Why should it be included in the others?

I just like to have it everywhere so Python parses the annotations consistently everywhere. Having that future enabled changes the runtime characteristics of annotations.

Should be done

theCapypara · 2024-07-12T14:37:11Z

skytemple/module/symbols/model_getter.py

+        get() instead.
+        """
+        if do_not_instantiate != self._instantiation_string:
+            raise ValueError("This class cannot be directly instantiated")


Its better to use Python's __new__ for a singleton pattern, but this is fine.

Elaborate, please? This is the only way I know right now.

https://pravash-techie.medium.com/python-singleton-pattern-for-effective-object-management-49d62ec3bd9b
"Using __new__"

skytemple/module/symbols/model_getter.py

skytemple/module/symbols/symbol_entry/symbol_entry.py

Frostbyte0x70 marked this pull request as draft June 19, 2024 23:35

theCapypara self-requested a review June 20, 2024 12:58

Frostbyte0x70 force-pushed the symbols branch 2 times, most recently from 9c6dce9 to 38702b3 Compare June 28, 2024 21:35

Frostbyte0x70 force-pushed the symbols branch from c26fc38 to e57288d Compare June 30, 2024 13:18

Frostbyte0x70 added 5 commits July 2, 2024 18:28

Add symbols editor

49b62c1

Fixes

7eb854d

More fixes

01b1a87

Refactor SymbolEntry and related classes

2559e07

Format

9bd634a

Frostbyte0x70 force-pushed the symbols branch from e57288d to 9bd634a Compare July 3, 2024 17:27

Fixes

7baef91

Frostbyte0x70 marked this pull request as ready for review July 3, 2024 17:59

theCapypara requested changes Jul 12, 2024

View reviewed changes

Frostbyte0x70 added 2 commits July 12, 2024 20:48

Address review comments

cf2fa26

Address review comments (2)

2fccc98

Frostbyte0x70 force-pushed the symbols branch from 1f2a4eb to 2fccc98 Compare July 18, 2024 21:37

theCapypara approved these changes Jul 19, 2024

View reviewed changes

theCapypara merged commit 948934e into SkyTemple:master Jul 19, 2024
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add symbols editor #761

Add symbols editor #761

Frostbyte0x70 commented Jun 19, 2024

Frostbyte0x70 commented Jun 29, 2024

theCapypara commented Jun 29, 2024 •

edited

Loading

Frostbyte0x70 commented Jul 8, 2024

theCapypara left a comment

theCapypara Jul 12, 2024

theCapypara Jul 12, 2024

Frostbyte0x70 Jul 12, 2024 •

edited

Loading

theCapypara Jul 12, 2024

Frostbyte0x70 Jul 12, 2024

theCapypara Jul 12, 2024

Frostbyte0x70 Jul 18, 2024

theCapypara Jul 18, 2024 •

edited

Loading

Frostbyte0x70 Jul 18, 2024

theCapypara Jul 12, 2024

Frostbyte0x70 Jul 12, 2024

theCapypara Jul 12, 2024

Frostbyte0x70 Jul 18, 2024

theCapypara Jul 12, 2024

Frostbyte0x70 Jul 12, 2024

theCapypara Jul 12, 2024 •

edited

Loading

Frostbyte0x70 Jul 18, 2024

Add symbols editor #761

Add symbols editor #761

Conversation

Frostbyte0x70 commented Jun 19, 2024

Frostbyte0x70 commented Jun 29, 2024

theCapypara commented Jun 29, 2024 • edited Loading

Frostbyte0x70 commented Jul 8, 2024

theCapypara left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Frostbyte0x70 Jul 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theCapypara Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theCapypara Jul 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theCapypara commented Jun 29, 2024 •

edited

Loading

Frostbyte0x70 Jul 12, 2024 •

edited

Loading

theCapypara Jul 18, 2024 •

edited

Loading

theCapypara Jul 12, 2024 •

edited

Loading