alpheios-project / alpheios-core

Alpheios Core Javascript Packages and Libraries
15 stars 2 forks source link

Lexical query fixes #555

Closed kirlat closed 3 years ago

kirlat commented 3 years ago

Fixes issues when the lexical query request status was not updated properly (see #547, #550). Also includes some other minor workflow improvements.

kirlat commented 3 years ago

May be we should define languageName the same way as languageID, languageCode, languageModel inside data-models and make a shotter path to it this way?

I agree that language handling became way to convoluted and needs a simplification. I think the new Language class should replace both languageID and languageCode. If we need a string representation of a language code, we can use the toCode() method of the Language. Maybe it even worth to create a code() alias to it so that it will read better (language.code() instead of language.toCode(), the latter one was created for serialization purposes and the name reflects that).

The language name, I think, is probably the part of the UI logic because it can differ depending on circumstances: the name of the same language can be displayed differently in different parts of the UI. It also might be localized. So I think the logic to determine the language name should belong to the UI layer, not to the more abstract data models. So maybe we can use the function within the app controller to return the language name, as currently, but make it to take the new Language object instead.

For legacy logic that still requires languageID we can use a method of the LanguageModel that will get the ID out of the Language class.

@balmas, @irina060981, what do you think about the plan? If it is usable, I can create an issue to track that.

kirlat commented 3 years ago
 const notFound = !homonym ||
      !homonym.lexemes ||
      homonym.lexemes.length < 1 ||
      homonym.lexemes.filter((l) => l.isPopulated()).length < 1

This part is about homonym, defines empty homonym and I believe could be used in different places in packages. It is better to place it as a check inside Homonym class.

Agree with this. Maybe we can call the method isEmpty()? @balmas, what do you think?

irina060981 commented 3 years ago

The language name, I think, is probably the part of the UI logic because it can differ depending on circumstances: the name of the same language can be displayed differently in different parts of the UI. It also might be localized.

I use it inside Alignment Editor and it is localized in my interface, not through data-models. And from my point of view - we could use here aliases with default name and message_name for l10n. But in most cases lat - is Latin and so on :) That's why I don't agree that it is really depend on UI logic.

kirlat commented 3 years ago
if (this._store.state.app.currentLanguageCode && this._store.state.app.currentLanguageName) {
        languageCode = this._store.state.app.currentLanguageCode
        languageName = this._store.state.app.currentLanguageName
      } else {
        languageCode = Constants.STR_LANG_CODE_UNDEFINED
        languageName = this.api.l10n.getMsg('TEXT_NOTICE_LANGUAGE_UNKNOWN')
      }

This part is about defining currentLanguageCode and currentLanguageName for the current state of the sesson and is not directly related to the homonym language data. I believe that these calculations should be scoped inside definition of currentLanguageCode and currentLanguageName.

I agree with this suggestion in general, but maybe we should postpone this because with the lexical query refactoring we would probably change the way the session is handled.

kirlat commented 3 years ago

do this (rename languageCode to langCode or change it inside message definition)

const targetWord = this._store.state.app.targetWord
const message = this.api.l10n.getMsg('TEXT_NOTICE_CHANGE_LANGUAGE',
          { targetWord, languageName, languageCode })

Great suggestion! I will update the code