Re: [PATCH] Add the capability to import and export settings

[Mónica Gómez]

Hello Yuchen,
On 24-02-04 01:50, Yuchen Pei wrote:
> Hi Mónica,
> On Tue 2024-01-30 15:04:22 -0600, Mónica Gómez wrote:
>
> > Signed-off-by: Mónica Gómez <eunbyeol64@naver.com>
> > ---
> >   NEWS                                          |  4 +
> >   html/preferences_panel/pref.js                | 85 ++++++++++++++++++-
> >   html/preferences_panel/preferences_panel.html | 10 ++-
> >   html/preferences_panel/prefs.css              | 16 ++++
> >   manifest.json                                 |  2 +-
> >   test/spec/LibreJSSpec.js                      | 43 ++++++++++
> >   6 files changed, 154 insertions(+), 6 deletions(-)
> Thanks a lot for the updated patch. The next two weeks I have less
> time to sit down at my computer for some uninterrupted time outside of
> work, so please bear with me with the actual review. Meanwhile I took
> a look at the patch and a few questions come to my mind that I would
> like to figure out (but if anyone has answers feel free to reply):

Thanks for your response.

> - how will this feature work with or be developed into incorprating the
>    other idea of having librejs read the config file for black and white
>    lists on startup?
This could definitely be adapted to read the config file on startup. It 
could be implemented as a separate function that reads the config file 
at the specified path or, to avoid repeating code, the `restoreSettings` 
function could read an argument with the route of the config file, and 
if such argument is `undefined` or `null` then it could show the open 
dialog. That way, we could use the same function to read the conf file 
on startup, and at the same the user could choose their own config file 
in case they want to change it, that then can be copied to the default 
LibreJS settings folder.

> - in your patch it reads and writes a conf file by manually parsing -
>   could it be better or necessary to use an existing library that
>   handles such files, or would it be easy to do the manual parsing now
>   and switch to a library when needed?
I wasn't sure about using an existing library because I was afraid of 
involuntarily making LibreJS less lightweight and/or less maintainable. 
For that I would appreciate if anyone could give their opinion about 
which is the best option.

> - external scripts are ok in the conf file as they are shown as "normal"
>   http(s) urls with some globbing patterns, like
>   https://forum.members.fsf.org/*
>   but inline scripts are represented in a funny way, like so
>
> --8<---------------cut here---------------start------------->8---
>   inline://www.fsf.org#(<SCRIPT>) // @license 
> mag…8855c4df2063479cf018bb81d75c6434c99b104fa0fcc40a45920792711650ec
> --8<---------------cut here---------------end--------------->8---
>
>   The above is what represents the inline scripts on the fsf homepage:
>
> --8<---------------cut here---------------start------------->8---
> // @license 
> magnet:?xt=urn:btih:1f739d935676111cfff4b4693e3816e664797050&dn=gpl-3.0.txt 
> GPL-3.0-or-later
>   var _paq = window._paq = window._paq || [];
>   /* tracker methods like "setCustomDimension" should be called before 
> "trackPageView" */
>   _paq.push(['trackPageView']);
>   _paq.push(['enableLinkTracking']);
>   (function() {
>     var u="https://piwik.fsf.org/";;
>     _paq.push(['setTrackerUrl', u+'matomo.php']);
>     _paq.push(['setSiteId', '5']);
>     var d=document, g=d.createElement('script'), 
> s=d.getElementsByTagName('script')[0];
>     g.type='text/javascript'; g.async=true; g.src=u+'matomo.js'; 
> s.parentNode.insertBefore(g,s);
>   })();
>   // @license-end
> --8<---------------cut here---------------end--------------->8---
>
>   will such lossy encoding reliably enough?
From the manual tests I made using IceCat and Firefox it seems to 
handle such inline scripts correctly. I suspect you mean that it might 
give some problems since inline scripts include characters like `#` and 
`//` that could be mistaken for comments. I was aware of that, so I 
implemented it in a way that only the lines that begin with `#` are 
considered as such, and if a given line includes `#` and `//` characters 
but they're not at the beginning then it is read as a normal URL. As I 
have never really worked with conf files in a development environment 
before, I'm not sure if what I did would be considered as a hack, or if 
it was the right way to implement it.

> - what do the tests cover? do they cover for example inline url cases?
As for now they just cover the act of creating a config file and then 
trying to read it. I struggled quite a bit in figuring out how to 
implement the tests, so I would appreciate a lot if anyone could give 
some feedback before adding the inline url cases.

--
--------------------------------------
Best regards,
Mónica Gómez <eunbyeol64@naver.com>
PGP Key: C6E176B5ED0712210C205917DE3142A1D89C649B
https://www.autumn64.xyz

OpenPGP_0xDE3142A1D89C649B.asc
Description: OpenPGP public key

OpenPGP_signature.asc
Description: OpenPGP digital signature