-
Notifications
You must be signed in to change notification settings - Fork 709
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
Double down on hooks, remove app plugin in favour of individual hooks for individual capabilities #12879
Draft
rtibbles
wants to merge
9
commits into
learningequality:develop
Choose a base branch
from
rtibbles:hooked_on_plugins
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
DEV: backend
Python, databases, networking, filesystem...
APP: Device
Re: Device App (content import/export, facility-syncing, user permissions, etc.)
APP: Learn
Re: Learn App (content, quizzes, lessons, etc.)
APP: Setup Wizard
Re: Setup Wizard (facility import, superuser creation, settings, etc.)
DEV: frontend
DEV: tools
Internal tooling for development
labels
Nov 25, 2024
rtibbles
force-pushed
the
hooked_on_plugins
branch
2 times, most recently
from
November 25, 2024 23:46
8da072b
to
3d53ffa
Compare
Build Artifacts
|
bjester
reviewed
Dec 17, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just posting some review-in-progress comments
kolibri/plugins/learn/assets/src/views/CurrentlyViewedResourceMetadata.vue
Show resolved
Hide resolved
kolibri/plugins/learn/assets/src/views/CurrentlyViewedResourceMetadata.vue
Show resolved
Hide resolved
rtibbles
force-pushed
the
hooked_on_plugins
branch
2 times, most recently
from
December 19, 2024 21:56
c1ec045
to
f7b44a9
Compare
Updated, rebased, and force pushed. |
Ensure we validate pythonpath option before settings option. Set python path for all CLI commands so that it can be used for plugins too.
Make the theme not break when no theme plugin is found.
Remove unneeded appCapabilities from kolibri/utils.
rtibbles
force-pushed
the
hooked_on_plugins
branch
from
December 20, 2024 20:12
f7b44a9
to
eae5b1a
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
APP: Device
Re: Device App (content import/export, facility-syncing, user permissions, etc.)
APP: Learn
Re: Learn App (content, quizzes, lessons, etc.)
APP: Setup Wizard
Re: Setup Wizard (facility import, superuser creation, settings, etc.)
DEV: backend
Python, databases, networking, filesystem...
DEV: frontend
DEV: tools
Internal tooling for development
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Goals
Summary
References
Depends on #12874 to allow specifying an extra python path for the development plugin
Needed to support learningequality/kolibri-installer-android#197
Reviewer guidance
I feel like leaning into the hooks makes sense to have a single way to customize Kolibri - is there anything about this that makes this less obvious?
By separating out the specific functionality that is more 'app' versus 'device' oriented, it has made each implementation a little more bespoke - but it seems sensible to me that metered connection checking doesn't have to be exclusive to app mode - is the minor added fiddliness an issue?
With the addition of the server hooks, we could also turn ZeroConf integration into a plugin, which would allow us to turn it off slightly more elegantly - and in a swappable way so that on Android we can provide the multicast implementation via built in Android mechanisms. The other configuration that feels like could benefit from being pluggable is the restart hooks - should we push more into plugins/hooks rather than relying on options.py?