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.

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

Add core extensions: UserCSS, UserJS #6267

Open
wants to merge 23 commits into
base: edge
Choose a base branch
from

Conversation

hkcomori
Copy link
Contributor

@hkcomori hkcomori commented Apr 6, 2024

Closes FreshRSS/Extensions#227

Changes proposed in this pull request:

  • Add UserCSS, which is based on CustomCSS and changes the file storage location to the user's directory
  • Add UserJS, which is based on CustomJS and changes the file storage location to the user's directory

How to test the feature manually:

  1. Confirmed that extensions can be enabled
  2. Confirmed that CSS and JS can be written from the settings page
  3. Confirmed that the written contents are saved in data/users/extensions/<extension-name>.
  4. Confirmed that the written contents can be read by reopening the settings page
  5. Confirmed that the written CSS and JS are referenced from <head>.

Pull request checklist:

  • clear commit messages
  • code manually tested
  • unit tests written (optional if too hard)
  • documentation updated

Additional information can be found in the documentation.

The name is used for the directory where the configuration
is stored and should not contain spaces.
Since the name was changed, I reset the version number and
changed to semantic versioning.
Changed the location of the configuration file to
the user data directory, because it is not `static`.
That way, the user's configurations are gathered
in the user directory, which makes it easier to backup them.
Remove procedures to install the extension
because it is no longer necessary.
Remove permission error indication because the storage location
is now in the user data directory managed by the application.
@Alkarex
Copy link
Member

Alkarex commented Apr 6, 2024

Thanks! We will first issue FreshRSS 1.24 relatively soon and then address bigger changes such as this one in the edge version towards the next release

@Alkarex Alkarex requested a review from Frenzie April 6, 2024 13:41
@Alkarex Alkarex mentioned this pull request Apr 6, 2024
4 tasks
@Alkarex
Copy link
Member

Alkarex commented Apr 6, 2024

I have done some refactoring. Please double-check.

- 1.0.0 change rules location to user data folder; promoted to core extension
- 0.0.1 initial version

## Examples
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could consider adding something in our documentation for core extensions

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, let's move the description to that document.
Please let me know a few things.

  1. Can I add it to here in the "Extensions" section?
  2. Is it acceptable to add only English, because I cannot use French?
  3. Can I reduce some of the configuration examples, as I don't think we need many of them?
  4. Can I remove the README for this extension?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated docs.

@Alkarex Alkarex requested a review from aledeg April 6, 2024 16:04

/** Return the user-specific, extension-specific, file content, or null if it does not exist */
protected final function getFile(string $filename): ?string {
$content = @file_get_contents($this->getExtensionUserPath() . '/' . $filename);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the future, we could try to see whether we could reduce the permissions for the extensions, so they can only call inherited functions

Version 0.0.1 will not be merged, so only version 1.0.0 will remain.
@hkcomori
Copy link
Contributor Author

hkcomori commented Apr 7, 2024

I have done some refactoring. Please double-check.

Thanks, nice refactoring.
I found some descriptions that are no longer used and I removed them.

I think it is a good idea to make some functions protected.
But I don't know if all of existing third-party extensions will not be broken.
Can we assume that they are guaranteed by unit tests?

@Alkarex
Copy link
Member

Alkarex commented Apr 7, 2024

Can we assume that they are guaranteed by unit tests

All the extensions of https://github.com/FreshRSS/Extensions are tested automatically, at least

@Alkarex
Copy link
Member

Alkarex commented Apr 7, 2024

I have made a new command to automatically test all known third-party FreshRSS extensions FreshRSS/Extensions#228

@@ -0,0 +1,8 @@
{
"name": "UserCSS",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we give a better name here?
Something like "Tweak UI with CSS" or "UI Tweaker"? Please with spaces instead of CamelCases.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not really mind. For me something with CSS in the name would be most obvious, such as "Custom CSS"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like CSS Tweaks and JS Tweaks, but User CSS / UserCSS and User JS / UserJS are decent enough names.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't mind if it is not named "UserCSS", but I did not put a space because this name will be used for the configuration file directory name.
I don't want to include spaces in any file paths.

Although the specifications will be changed, could we use $this->getEntrypoint instead of $this->getName() for the configuration file directory name?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hkcomori That is a reasonable suggestion, thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some AI suggestions:

  • CSS Customizer
  • CSS Personalizer
  • Flexible CSS Tuner
  • CSS Overwriter
  • CSS Remixer

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those suggestions are worse to much worse imho. :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I changed the directory name to entrypoint and added a space to the name.

return USERS_PATH . "/{$username}/extensions/{$this->getName()}";
return USERS_PATH . "/{$username}/extensions/{$this->getEntrypoint()}";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this cause a new entry in the update.php that changes existing paths?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could, if anyone is motivated in doing so. Might be a bit tricky to do safely, though. Another option could be to have a migration code in

abstract public function init();

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we expect version jumps, I think it is better method suggested by @Alkarex.
But we can't have a common migration code in init(), because it will be overwritten by each extension.
So how about adding a new method that is called just before init()?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, the extensions should call the parent method parent::init() when overriding. We could fix those we know like I did in FreshRSS/Extensions#228

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, then I will implement the migration code in init().

Copy link
Contributor Author

@hkcomori hkcomori Apr 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was also necessary to add migration code in handleConfigureAction() to support the use case of opening the configuration view before enabling the extension.

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

Successfully merging this pull request may close these issues.

[Feature] Save CustomCSS settings to user data
4 participants