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

Editor: share base menu functionality between ScriptEditor and DialogEditor #1743

Conversation

ivan-mogilko
Copy link
Contributor

@ivan-mogilko ivan-mogilko commented Jul 23, 2022

Resolves #1480

The problem with DialogEditor is that it's a stripped copy-paste version of the ScriptEditor (when it comes to working with script itself). So many menu commands that work in the ScriptEditor control are not available in DialogEditor.

The solution seem to be to create a parent class that shares a chosen functionality among these two.

In this PR, the parent class ScriptEditorBase is created, which has following functionality picked out from ScriptEditor and DialogEditor and merged:

  • Menu commands and their functions;
  • Context menu commands for Scintilla control and their functions;
  • Various operations on a script: find, replace, goto line, goto definition, etc.

The menu commands appear to work per se, but some of them have exceptions where they don't work. This is not related to command itself, but to internal functionality of the editor class. For instance, DialogEditor does not support AutoCompleteData for some reason (maybe because it's more complicated to parse dialog scripts). Because of that neither autocomplete nor "go to definition" will work on variables declared locally in dialog script. I tried to investigate this, but it seems like implementing this for dialogs would require picking out much more code from the ScriptEditor, and it's not clear how well that will work. So I'd rather leave that for future consideration.

@ivan-mogilko ivan-mogilko added what: editor related to the game editor context: code fixing/improving existing code: refactor, optimise, tidy... context: ui/ux context: dialogs dialogs and dialog script labels Jul 23, 2022
@ericoporto
Copy link
Member

I wonder if the autocomplete issues would be fixed after a rebase, since #1726 was made and merged shortly after this.

@ivan-mogilko
Copy link
Contributor Author

ivan-mogilko commented Jul 31, 2023

I wonder if the autocomplete issues would be fixed after a rebase, since #1726 was made and merged shortly after this.

I had plans to rebase this for 3.6.1, but autocomplete problems mentioned in this ticket will not be fixed, because #1726 did not address them in particular:

DialogEditor does not support AutoCompleteData for some reason (maybe because it's more complicated to parse dialog scripts). Because of that neither autocomplete nor "go to definition" will work on variables declared locally in dialog script.

The ScriptEditorBase parent class implements menu commands shared between standard ScriptEditor and DialogEditor.
Copied code mostly from the ScriptEditor, as its list of commands is more full.
Few commands that are not applicable in DialogEditor (at least not yet) are added in override methods of ScriptEditor.
Moved common operations (search, goto line, etc) to ScriptEditorBase.
@ivan-mogilko
Copy link
Contributor Author

ivan-mogilko commented Aug 10, 2023

Recreated for 3.6.1, using old pr as a reference, because some ScriptEditor code could have changed since.

Requires testing.
EDIT: at least I did not make it crash... yet.

@ivan-mogilko ivan-mogilko marked this pull request as ready for review August 10, 2023 21:32
@ericoporto
Copy link
Member

ericoporto commented Aug 11, 2023

Testing with simple scripts, go-to definition looks like it's working. The text above of the PR mentions the following

Because of that neither autocomplete nor "go to definition" will work on variables declared locally in dialog script.

Yes, this is true, but I had to read the issue above to figure this was a thing because ultimately I rarely declare variables locally in a dialog script, but instead mostly call functions that are either global or in objects defined elsewhere (like a character).

Overall I really like this PR because it adds a lot of new functionality to the dialog editor without increasing code lines as it's just picking features from elsewhere and sharing it smartly.

@ivan-mogilko
Copy link
Contributor Author

Yes, it's a quite straightforward change, should have added this to 3.6.0, but i did not expect it to be released about 6 months later.

@ivan-mogilko ivan-mogilko merged commit ec3062e into adventuregamestudio:master Aug 13, 2023
17 checks passed
@ivan-mogilko ivan-mogilko deleted the 360--dialogscriptcontextmenu branch August 13, 2023 01:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
context: code fixing/improving existing code: refactor, optimise, tidy... context: dialogs dialogs and dialog script context: ui/ux what: editor related to the game editor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Go to definition" feature in dialog editor
2 participants