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

refactor: reconstruct option management #659

Merged
merged 18 commits into from
May 17, 2023
Merged

Conversation

wxharry
Copy link
Collaborator

@wxharry wxharry commented Apr 23, 2023

Brief Information

This pull request is in the type of (more info about types):

  • build
  • ci
  • docs
  • feat
  • fix
  • perf
  • refactor
  • test

Related issues (all available keywords):

Details

  • use val-loader to get imported features from here
  • refactor options management
    • replace class with functions
    • add all imported features to the option list
    • control on/off from the feature-manager
  • use webext-options-sync-per-domain to improve the option management (?)
  • need consistent feature ID, especially for locale
  • cache related
  • docs and comments

Checklist

Others

@menbotics menbotics bot added the kind/enhancement Category issues or prs related to enhancement. label Apr 23, 2023
@wxharry wxharry changed the title [WIP] refactor option management [WIP] refactor: reconstruct option management Apr 23, 2023
@l1tok
Copy link
Collaborator

l1tok commented Apr 25, 2023

Nice work! It works as soon as I cancel the option button,but there is a small problem that all the options are re-selected after refreshing.

Settings.-.Google.Chrome.2023-04-25.20-34-27.mp4

@wxharry
Copy link
Collaborator Author

wxharry commented Apr 25, 2023

Nice work! It works as soon as I cancel the option button,but there is a small problem that all the options are re-selected after refreshing.

Thank you for pointing that out. Since this pr is still WIP, i will fix it in the next few commits.

@tyn1998
Copy link
Member

tyn1998 commented May 3, 2023

Hi @wxharry, I have learnt your code especially the "importedFeatures" part and I totally figured out the way you extracted the imported features from index.ts and injected them into a webpack module during build.

I learnt a lot about webpack loaders as well, finding that val-loader is not so necessary and features-loader.js should not be treated as a webpack module, rather it should be used as a webpack loader as its name suggests.

So I changed the code a bit, treating README.md as a module (this is how rgh does, though for now we don't extract feature list from README.md), and used features-loader.cjs to convert the README.md module so its content will be export const importedFeatures = ... after being processed by the loader. Then we can import {importedFeatures} from 'path/of/README.md' anywhere in code.

@tyn1998
Copy link
Member

tyn1998 commented May 4, 2023

There is still a lot of room for code improvement, working in progress...

src/utils/settings.ts Outdated Show resolved Hide resolved
@tyn1998
Copy link
Member

tyn1998 commented May 14, 2023

Actually, what I did last week (those 4 commits) didn't work right because I found several checked checkbox would be unchecked after page refresh this morning.

However hours work later, I hope the option manager now works. I did:

  • refactored a more cohesive options-storage.ts (a single instance and be consumed everywhere, this time I vote for Class rather several functions)
  • to use multiple keys to store all options, rather a single "settings" key whose value is a serialized stirng
  • use chrome.storage.sync.set/get so options can be synced by google account. (we can pass [default options via get(defaults))
  • other changes

More info for options-storage.ts:

Currently this file is bundled into ContentScript and Options since both require it to access option storage. ContentScript only read options, while Options read and write options.

I mentioned single above to stress that one application only needs to instantiate its configuration class (in our case, i.e. OptionsStorage) once rather multiple times as old Hypercrx did before. In fact there are two instances of OptionsStorage class for both CS and Options, because CS and Options run in different context so they are independent in terms of runtime environment.

Hi @lhbvvvvv, could you help to test this PR again?

@l1tok
Copy link
Collaborator

l1tok commented May 15, 2023

On my personal computer, the feature oss-gpt is missing whether I select it on the options list or not.

Also, I think perceptor-layout, perceptor-tab, and repo-networks can be combined into one option. The Perceptor button at the top is still displayed when the perceptor-layout and repo-networks options are not selected, however perceptor-tab option works well.
image

Except for those mentioned above, everything is working fine!

@wxharry wxharry changed the title [WIP] refactor: reconstruct option management refactor: reconstruct option management May 15, 2023
@tyn1998
Copy link
Member

tyn1998 commented May 16, 2023

@lhbvvvvv Thank you for your test! I know what causes the bug and will fix it ASAP.

Also, I think perceptor-layout, perceptor-tab, and repo-networks can be combined into one option.

Considered this, but maybe improve it later :) thanks for your suggestion!

@tyn1998
Copy link
Member

tyn1998 commented May 16, 2023

@lhbvvvvv fixed it, please verify if my patch works.

Hi @wxharry, could you give a review on my code?

@wxharry
Copy link
Collaborator Author

wxharry commented May 16, 2023

Hi @wxharry, could you give a review on my code?

LGTM

@l1tok
Copy link
Collaborator

l1tok commented May 17, 2023

@lhbvvvvv fixed it, please verify if my patch works.

It ran successfully without problems.

Copy link
Collaborator

@l1tok l1tok left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you @wxharry @tyn1998

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement Category issues or prs related to enhancement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants