UI controller refactoring phase 3 #481
Conversation
| this.isDeactivated = false | ||
|
|
||
| this.activateModules() | ||
| // this.state.activate() |
There was a problem hiding this comment.
I just forgot to remove it. It has been moved to the UI controller and is no longer needed: https://github.com/alpheios-project/alpheios-core/blob/dev-ui-controller-refactoring-3/packages/components/src/lib/controllers/ui-controller.js#L131. Will clean it up
| /* uiSetFontSize (uiOptions) { | ||
| const FONT_SIZE_PROP = '--alpheios-base-text-size' | ||
| try { | ||
| document.documentElement.style.setProperty(FONT_SIZE_PROP, | ||
| `${uiOptions.items.fontSize.currentValue}px`) | ||
| } catch (error) { | ||
| this.logger.error(`Cannot change a ${FONT_SIZE_PROP} custom prop:`, error) | ||
| } | ||
| } | ||
| } */ |
There was a problem hiding this comment.
Yes, just forgot about this one.
| if (this._uiState.isPanelStateDefault() || !this._uiState.isPanelStateValid()) { | ||
| this._uiState.setPanelClosed() | ||
| } | ||
| if (this._uiState.isPanelOpen()) { | ||
| this.openPanel(true) | ||
| } |
There was a problem hiding this comment.
this logic is a little different than before. is there any chance the panel will be left in an undefined state?
There was a problem hiding this comment.
I thought it is an equivalent but I might be missing something.
In master, it is:
if (this.state.isPanelStateDefault() || !this.state.isPanelStateValid()) {
this.setDefaultPanelState()
}
// If panel should be opened according to the state, open it
if (this.state.isPanelOpen()) {
if (this.uic.hasModule('panel')) { this.api.ui.openPanel(true) } // Force close the panel
}
and as setDefaultPanelState() is defined as
setDefaultPanelState () {
if (!this.uic.hasModule('panel')) { return this }
this.state.setPanelClosed()
return this
}
Then, combined, it will be:
if (this.state.isPanelStateDefault() || !this.state.isPanelStateValid()) {
if (!this.uic.hasModule('panel')) { return this } // I.e. do nothing
this.state.setPanelClosed()
}
// If panel should be opened according to the state, open it
if (this.state.isPanelOpen()) {
if (this.uic.hasModule('panel')) { this.api.ui.openPanel(true) } // Force close the panel
}
In both cases nothing is done if there is no panel module available. We open a panel if its state is open and close if the state is either default or invalid. In all other cases we do nothing. So I think it must be pretty much what we had before.
The complicated part is that the state is provided from an outside of an app: both webextension and embed lib have their own state object. That bothers me because an app controller cannot know what possible states will it be dealing with. Because of this we, in the app controller, cannot expect anything about the states being provided. I think this is no good.
That's one of the reasons I was suggesting to simplify state management. I as thinking about using Vuex but I agree it's not a very good idea because of a dependency that it will create. But I still think something can and needs to be done here. Maybe we can do the change when we'll convert to code to adapt to the Manifest V3, because it's a webextension that using the state the most?
What do you think about that? Can you suggest any improvements to the current piece of code?
There was a problem hiding this comment.
I think the current code is fine for now.
I agree that we need to deal with the co-managed state (client app vs app controller) better. Not sure when or how is best to do that right not, but for the moment waiting until we adapt for Manifest 3 seems as good of an idea as any!
There was a problem hiding this comment.
I compared AppController and UIController manually and I have some notices for the structure, may be it would be useful.
For now we have the following controllers:
- AppController (this controller should be used directly in mebed-lib, webextension and may be in future in some other applications)
-
UIController (this controller should unite parameters and methods for handling with html components and I belive in future we should be able to use some subclasses of it in other applications)
-
UIEventController (this controller gives methods to work with html events - handleEscape, getSelected, mouseMove)
Here I think with such division we couldn't solve the problem of reducing dependency between AppController, because we have here- cross-register events with dependency in lexis
- we have cross-dependency tabs, queryParams, platform
- also we have activate/deactivate on page mathods, that could be used only with HTML layout and could not be avoided
and for now we have a very spread code for defining options/settings that is tightly connected with AppC and UIC - may be it is worth to define OptionsController too, that could be used different ways - with dependency on remote, local storage or only default usage?
also about Vuex store, I believe that it is worth to move it to a separate StorageController that could be / or in future could be not used inside other controllers, and may be it could have different Controllers - a separate for apps with UI and without
and a small notice here we have three variants of similiar data structures:
- modules inside AppC
- modules inside UIC
- api inside AppC
they have similiar names but different business logic, may be it is worth to redefine names, because
- modules inside AppC -
- lexis - for lexicalQuery
- l10n - for translation messages
- modules inside UIC
- panel
- popup
- api inside AppC
and here we have the mix of the all this bussiness logic
Also we have the following controllers here
- UserDataManager
- WordlistController
They also have dependencies accross UI and different storage ways and may be it is worth to clear such dependencies too and pass them as parameters via variables
From my point of view we should try to separate this controller functions to small parts that could be turn on/turn of in different cases.
It could be done via variables or via events, that are passed between controllers.
And also I think that we should search for the solution to place all events in one place, this could help in future to track and check them easier.
|
@irina060981, thanks for you thought and comments! These are good ideas and suggestions, and I will definitely need to think about it for a while. We could discuss them during the call tomorrow. |
In this step, I moved a majority of UI-related functions into a UI controller. It will require an update of webextension and embed-lib. I will submit a PR for those once this PR will be merged.
I understand that it might be hard to review the changes because there will be no comparison shown between an old and a new version. But I think maybe you will be able to check for some tricky conditions and/or checks, or any specific business logic cases that I might have twisted inadvertently. It's good to be sure that there are no unexpected changes caused by this update.
In the next step I'm planning to move a
uimodule of Vuex into the UI controller. I will also review a remaining UI-related functionality in the App controller and will decide what can be moved to the UI controller and what can be simplified or eliminated.Thanks!