Skip to content
This repository has been archived by the owner on Feb 25, 2023. It is now read-only.

Bug with termTags and Translator data format #337

Closed
toasted-nutbread opened this issue Jan 28, 2020 · 10 comments · Fixed by #878
Closed

Bug with termTags and Translator data format #337

toasted-nutbread opened this issue Jan 28, 2020 · 10 comments · Fixed by #878
Labels

Comments

@toasted-nutbread
Copy link
Collaborator

There is currently a bug with termTags not being shown properly when result grouping mode is merge, causing the tags to never be shown. The issue stems from the termTags not being consistently defined, and DisplayGenerator is reading the property from the wrong object.

DisplayGenerator._appendMultiple(expressionsContainer, this.createTermExpression.bind(this), details.expressions, [details]);
DisplayGenerator._appendMultiple(reasonsContainer, this.createTermReason.bind(this), details.reasons);
DisplayGenerator._appendMultiple(frequenciesContainer, this.createFrequencyTag.bind(this), details.frequencies);
DisplayGenerator._appendMultiple(definitionsContainer, this.createTermDefinitionItem.bind(this), details.definitions, [details]);

Above, DisplayGenerator.createTermExpression will read termTags from details.expressions[n] in merge mode, and from details otherwise. However, in merge mode, termTags is defined on details, while all of the other data used to generate the expression is still on details.expressions[n].

In my opinion, there is a larger issue at hand here where the data returned from the translator backend returns an inconsistent format, a format which changes based on the presentation mode. The termTags that is being read by DisplayGenerator.createTermExpression can be either null or an array, which is usually empty, which is another inconsistency.

It would be nice if these data structures could be standardized, but this also presents several problems. The biggest issue I see is the handlebars templates used to generate the Anki fields. These templates rely on the current data format, so if the internal data structure is updated to be more consistent, this format would still need a way to be converted to the old format to provide backwards compatibility with Anki template generation.

However, in the long run, this is probably worth it. It's somewhat difficult to figure out what some of the logic in translator.js does due to this issue, as well as some confusing naming conventions (deinflections, definitions, definitions.definitions, expressions.expressions, etc.) Now that HTML templates are used for display content generation, it should also be easier to update the internal data format, as all of the presentation styling is handled using CSS now.

I'll look into a way to fix the tag bug in the short term, but as a longer term goal, a standardized data format may make development and debugging easier.

@siikamiika
Copy link
Collaborator

There is currently a bug with termTags not being shown properly when result grouping mode is merge

Do you mean when it is not merge? I can reproduce with 少ない, but アジア works.

I agree that the data format is problematic, but if I remember correctly it was decided in #84/#95 to be the easier way to move forward. I should have probably considered it technical debt to be fixed as soon as possible after the feature had been deployed. Now there are special cases everywhere causing bugs like #336.

Do you think it would make sense to replace

expression
reading
furiganaSegments
termTags
expressions

with a terms array that contains everything currently in expressions in merge mode, and move

definitionTags
dictionary

inside definitions at all times? source and rawSource are more tricky.

@siikamiika siikamiika added the bug label Jan 28, 2020
@toasted-nutbread
Copy link
Collaborator Author

I don't see term tags when results grouping is merge.

output

@siikamiika
Copy link
Collaborator

It seems to depend on which mode you have enabled when you import the dictionaries. I re-imported with resultOutputMode = group and now it works like yours.

@siikamiika
Copy link
Collaborator

(I also noticed that "Enable search" isn't checked automatically for imported dictionaries although they're enabled)

@toasted-nutbread
Copy link
Collaborator Author

(I also noticed that "Enable search" isn't checked automatically for imported dictionaries although they're enabled)

I think this is a bug where the checkbox isn't updated properly after the import. Refresh the page after importing and it is should appear checked (without doing it yourself).

@siikamiika
Copy link
Collaborator

It seems to depend on which mode you have enabled when you import the dictionaries. I re-imported with resultOutputMode = group and now it works like yours.

Actually this was caused by having the wrong main dictionary. Now my dictionaries work with all three modes after the re-import. Still doesn't explain why re-import fixed group mode. When did you last reinstall the dictionaries? For me it was probably a month or two ago.

@siikamiika
Copy link
Collaborator

c2ff25b could be relevant

@toasted-nutbread
Copy link
Collaborator Author

I reimported fairly recently, as I wind up purging my dev environment fairly often. Here's a few things I've noticed:

Install two primary dictionaries (I tested with JMdict + KireiCake). Leave one enabled and disable the other. Set the main dictionary to the disabled one. Scan a word with termTags in merge mode, and no tags will show up. Change the main dictionary to the enabled one and then scan the word again, the term tags should appear.

I don't know how my previous settings were causing it to not work (the settings from the video), as I messed around a bit and then it started working. The primary dictionary was enabled for that also.

One thing I have done recently was import the settings from #327 (comment), but I updated the main/enabled dictionaries after doing that.

@siikamiika
Copy link
Collaborator

#338 (comment)

@siikamiika
Copy link
Collaborator

I ran into the issue with missing tags again when testing with resultOutputMode as group. I didn't investigate much further, but it seems that the termTags are determined by the dictionary that matches first. So if you sort some J-J without termTags before JMdict, there are no tags.

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

Successfully merging a pull request may close this issue.

2 participants