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

Automatically add character names to dictionary #839

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from

Conversation

dmjohnsson23
Copy link

This is still a work in progress. There is an issue in that newly added character names don't seem to immediately be recognized; they still are underlined red and marked as spelled incorrectly. I think this issue is probably because highligher.rehighlight() needs to be called for the text view(s) after the name is added to the dictionary. Trouble is, I'm not sure the best way to get a reference to that from the context of the spellchecker.

I will continue working on this as time permits. Comments are welcome.

That was the easy part. Now I have to find a good place in the code base when the character models are loaded, as well as the spellcheck, to pipe the data in to the dictionary...
@TheJackiMonster
Copy link
Collaborator

You are right about the highlighter. I would recommend looking at the code of the context menu in manuskript/ui/textEditView.py which also adds custom words to the dictionary.

However in your case, you should probably consider using toggleSpellcheck(...) in manuskript/mainWindow.py which should recursively trigger all textEditViews to update their highlighting. Otherwise if you find a better solution for this problem, I wouldn't mind some better structure for accessing such globally used objects as the spellchecker or the dependent highlighter (The highlighters are currently all separate instances per textEditView of type BasicHighlighter).

@dmjohnsson23
Copy link
Author

I feel like this is ready to be reviewed. I have tested it on my machine and everything seems to be working.

@TheJackiMonster
Copy link
Collaborator

I will review this very soon. You can already try to squash the commits down to one and rebase to the main branch if necessary. So merging won't make any problems.

@TheJackiMonster
Copy link
Collaborator

Ok, I have looked at the code and how it's working. I think technically it works fine but there are still some problems which should be fixed, I think:

  • Let's assume I have a character containing a word in its name which I added some time ago as custom word. Renaming the character would still remove the word which is unexpected behavior for a user.
    • As fix I recommend using a separate list or dictionary (a dictionary has probably the faster memory access for this usecase) instead of adding names into the custom words list. (Actually I'm not even sure why we use a list instead of a dictionary here for custom words either... ^^')
  • Another problem is performance. We already have a certain performance penalty in Manuskript overall but I would like to reduce that with upcoming changes. So currently with your changes, renaming a character has got quite some input lag because of the many lookups and memory accesses (probably the removal of old words and reallocations cause this problem). A dictionary instead of a list will help with this mostly as well.

Otherwise I really like the comments and the event usage. But maybe also rename 'onDictionaryChanged' and 'onCharacterModelChanged' since they don't represent bound events but methods to change attributes. Maybe even remove the names from the old (spellcheck-)dictionary if it changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants