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.

Sign up for GitHub

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

Jump to bottom

generalized manual loaded modules mechanism #1135

Merged
merged 1 commit into from
Mar 27, 2022
Merged

generalized manual loaded modules mechanism #1135

merged 1 commit into from
Mar 27, 2022

Conversation

spleen1981
Copy link
Contributor

No description provided.

@spleen1981 spleen1981 merged commit 250fe65 into qtranslate:master Mar 27, 2022
@spleen1981 spleen1981 deleted the ma_modules branch March 27, 2022 00:31
@herrvigg
Copy link
Collaborator

Good initiative! I had this on my todo-list.
As you probably noticed, I added you as member of the qTranslate organization :) So you could merge directly. But I'd like to review such Pull Requests, especially when impacting core functionalities. There's some WIP (Work In Progress) stuff and I have a few things in mind. I added a required approval in the config, but I don't know if this also applies to org members with write permission. Let's see.

@herrvigg herrvigg added the core Core functionalities, including the admin section label Mar 27, 2022
@herrvigg
Copy link
Collaborator

My idea was to have some menu/switches in the main list, so any module can be disabled. But it can certainly evolve from this change.

@spleen1981
Copy link
Contributor Author

Ok just tried with an harmless commit (f363fd1) and it works as follows:
image

For next commits I will be waiting your review (my bad, I would have done like that in the first place...)

@spleen1981
Copy link
Contributor Author

My idea was to have some menu/switches in the main list, so any module can be disabled. But it can certainly evolve from this change.

Makes sense, let me take a look.
Also some other structural modifications are needed in order to avoid hardcoding modules list/definitions in core files.
Definitions should be included in the single module folder itself following a scheme defined in core files along with the defaults, and list should be created based on mudules folder content. I will propose an implementation.
Other things will come up to mind while working I guess...

@spleen1981
Copy link
Contributor Author

My idea was to have some menu/switches in the main list, so any module can be disabled. But it can certainly evolve from this change.

Makes sense, let me take a look.

maybe something like #1137

@spleen1981 spleen1981 mentioned this pull request Apr 2, 2022
Merged
@herrvigg herrvigg added the modules Related to modules, internal to QTX label Apr 10, 2022
herrvigg added a commit that referenced this pull request Apr 18, 2022
@herrvigg
Refactor bool-array setting to QTX_BOOLEAN_SET
2c65a0d 
The `bool_elements_array` setting was introduced in #1135 and #1137.
Re-using the QTX_ARRAY code with an extra argument makes it hard
to maintain. The QTX_ARRAY type is not suited for set of checkboxes.

Separate the logic by handling this type all separately.
The `QTX_BOOLEAN_SET` was legacy, defined but not used in the code.
Make the new type generic though only used by `ma_module_enabled`.
Remove the extra `bool_elements_array` argument.
@herrvigg herrvigg mentioned this pull request Apr 18, 2022
Merged
herrvigg added a commit that referenced this pull request Apr 18, 2022
@herrvigg
Refactor bool-array setting to QTX_BOOLEAN_SET (#1151)
ea83b64 
The `bool_elements_array` setting was introduced in #1135 and #1137.
Re-using the QTX_ARRAY code with an extra argument makes it hard
to maintain. The QTX_ARRAY type is not suited for set of checkboxes.

Separate the logic by handling this type all separately.
The `QTX_BOOLEAN_SET` was legacy, defined but not used in the code.
Make the new type generic though only used by `ma_module_enabled`.
Remove the extra `bool_elements_array` argument.
spleen1981 added a commit to spleen1981/qtranslate-xt that referenced this pull request Apr 18, 2022
@spleen1981 @herrvigg
Modules activation (#10)
55fe535 
* Refactor module activation handler and options

Disambiguate the confusion between module state and admin options.

The module state depends in general on other plugin states.
The new admin options to manually disable or enable the modules are
just preferences for the module activation. They should be stored
regardless of the current state and can be deleted at any time.

Rename the option to `modules_ma_enabled` and make it admin option.
Refactor the module state update to separate the concepts.

* Minor cleanup

* Fix missing checkbox, cleanup module info

* Refactor bool-array setting to `QTX_BOOLEAN_SET` (qtranslate#1151)

The `bool_elements_array` setting was introduced in qtranslate#1135 and qtranslate#1137.
Re-using the QTX_ARRAY code with an extra argument makes it hard
to maintain. The QTX_ARRAY type is not suited for set of checkboxes.

Separate the logic by handling this type all separately.
The `QTX_BOOLEAN_SET` was legacy, defined but not used in the code.
Make the new type generic though only used by `ma_module_enabled`.
Remove the extra `bool_elements_array` argument.

* Use `QTX_BOOLEAN_SET` for module checkboxes

* Cleanup dead code

* Fix module init on activation, complete doc

* Rename `ma_enabled_modules` to `admin_enabled_modules`

Co-authored-by: HerrVigg <herrvigg@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
core Core functionalities, including the admin section modules Related to modules, internal to QTX
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@spleen1981 @herrvigg