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

Various Improvements #379

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Various Improvements #379

wants to merge 11 commits into from

Conversation

NyeUsr
Copy link

@NyeUsr NyeUsr commented Dec 31, 2023

Mobile Compatibility and Usability:

  • Update HTML and CSS files so they format properly on Android (Firefox)
  • Update popup.js so the popup closes when clicking "Edit Redirects" on Android
  • Change options_ui in manifest so it works as expected (especially on mobile)

Import Functionality Enhancements:

Rule Adjustments:

  • Make the example rule use HTTPS as now most browsers upgrade to HTTPS by default
  • Fix example rule not working on install (fix Builtin Example Doesn't Work #356)
  • Move Duplicate button to a spot where it makes more sense
  • Fixed Base64 decode

Code Readability and Efficiency:

  • Clean up HTML, CSS & JS files so they're easier to read
  • Improve readability and function of importexport.js (modernize, fix redundancy, remove example rule when importing more than 1 rule)
  • Update the rules logic to use a for...of loop for improved simplicity and readability
  • Use template literals to improve readability
  • Replace depreciated unescape code in redirect.js

Additional Changes:

  • Added new message to the footer in redirector.html about Einar's passing
  • Update http: links to https: in help.html

As of the last update 16/04/2024

* Update HTML and CSS files so they format properly on Android (Firefox)
* Update popup.js so the popup closes when clicking "Edit Redirects" and Android
* Make the example rule use HTTPS as now most browsers upgrade to HTTPS by default
* Change options_ui in manifest so it workss as expected (especially on mobile)
* Clean up HTML, CSS & JS files so they're easier to read
* Update importRedirects function in importexport.js so it uses array methods for improved simplicity and readability
* Update the rules logic to use a for...of loop for improved simplicity and readability
* Use template literals to improve readability
* improve readability and function of importexport.js (modernize, fix redundancy, remove example rule when importing more than 1 rule)
* Fix example rule not working on install
* More minor CSS consistency updates
@NyeUsr
Copy link
Author

NyeUsr commented Jan 2, 2024

@Gitoffthelawn Tested with Brave, Librewolf, and Firefox Nightly on Ubuntu 23.04, 23.10, and NixOS 23.11 as well as Fennec F-Droid v121.0.0 on Android 14. My 43 rules have had zero issues.

@Gitoffthelawn
Copy link
Collaborator

Thank you. Would it be possible for you to test on desktop Firefox ESR as well (currently at v115)?

@NyeUsr
Copy link
Author

NyeUsr commented Jan 2, 2024

Thank you. Would it be possible for you to test on desktop Firefox ESR as well (currently at v115)?

Of course, I'll ping you again after I use it for another at least 12 hours between browsers and computers.

@Gitoffthelawn
Copy link
Collaborator

@NyeUsr Thanks. Take your time. I have a busy week scheduled...

* Import now updates pre-existing rules instead of making duplicates
* Minor bug fix on mobile
* Update http: links to https: in help.html
@NyeUsr
Copy link
Author

NyeUsr commented Jan 3, 2024

Import now updates pre-existing rules instead of making duplicates which should fix #339 . In my testing everything continues to work wonderfully. Currently it checks the description and if a rule with the description matches it updates the rule, anyone see any issue or anything I may have missed with this? @Gitoffthelawn @KaKi87

* Duplicate Button now increments numbers in the description to help prevent the possibility of import issues
@KaKi87
Copy link

KaKi87 commented Jan 3, 2024

it checks the description and if a rule with the description matches it updates the rule, anyone see any issue or anything I may have missed with this?

Sorry but that's definitely bad : what would happen I change a rule's description ? 😅

You must use something that the user cannot change, i.e. an ID.

@NyeUsr
Copy link
Author

NyeUsr commented Jan 3, 2024

it checks the description and if a rule with the description matches it updates the rule, anyone see any issue or anything I may have missed with this?

Sorry but that's definitely bad : what would happen I change a rule's description ? 😅

You must use something that the user cannot change, i.e. an ID.

Interesting. I hadn't thought about you changing the description of a redirect. Guess I always treated it as a title lol. I'll see what I can figure out.

@KaKi87
Copy link

KaKi87 commented Jan 3, 2024

It is a title. And a title is user-editable.

@NyeUsr
Copy link
Author

NyeUsr commented Jan 3, 2024

It is a title. And a title is user-editable.

Yeah. The ID would be stored in the JSON file, which is less likely to be edited by the user. Currently, I believe the issue with either solution is that if you choose to import content from another user, which I often do, it could potentially overwrite one of your redirects if they end up having the same ID or name. Although this scenario is relatively unlikely I would prefer to err on the side of caution. As part of my planned update to the import feature in general, which is a larger project requiring more time, I can address this. I intend to add a functionality to handle importing from URLs, making it more convenient to import examples and community rules without the need for manual copy-pasting. Within this new import menu, I can include a checkbox for merging or updating rules based on title and/or ID. What're your thoughts on this approach?

* Improvements to the UI on mobile
* Fix import button not working more than once per page load
@KaKi87
Copy link

KaKi87 commented Jan 4, 2024

it could potentially overwrite one of your redirects if they end up having the same ID

That cannot realistically happen with IDs as long as you're using nanoid or uuid.

I can include a checkbox for merging or updating rules based on title and/or ID.

No, only ID must be used, there is no other choice to be.

@NyeUsr
Copy link
Author

NyeUsr commented Jan 4, 2024

That cannot realistically happen with IDs as long as you're using nanoid or uuid.

I can include a checkbox for merging or updating rules based on title and/or ID.

No, only ID must be used, there is no other choice to be.

Sorry, should've updated that or made a new post. I've been testing using nanoid since before your post. I honestly don't know why I included 'based on title and/or ID.' either since before I even sent that I was looking to use ID only. As always I appreciate the replies, thank you!

* New import popup allowing both import from file and import from url
* Unique rule IDs based on Nano ID
* Rules without an ID will be automatically assigned an ID (for upgrading users and also applies to imports)
* Fixed Base64 decode
* Replace depreciated unescape code in redirect.js
@NyeUsr
Copy link
Author

NyeUsr commented Jan 10, 2024

I've been testing this newest version for the past couple days and it seems to work very well. Anyone else able to test it?

@eikaramba
Copy link

looks good for me. @Gitoffthelawn are you still maintaining this project?

@KaKi87
Copy link

KaKi87 commented Apr 16, 2024

Anyone else able to test it?

Oh I'm sorry I forgot to, I will do shortly !

* Update renderRedirects() to reduce DOM manipulations
* Minor CSS consistency updates
-
Been sitting on these for a while now...
@Gitoffthelawn
Copy link
Collaborator

looks good for me. @Gitoffthelawn are you still maintaining this project?

Yes, however I don't currently have what I need to release a new version. Let me see if I can arrange to get everything I need.

@throwaway242685
Copy link

throwaway242685 commented Aug 30, 2024

I think this PR is really needed for Android.

are there any plans to merge it?

@Jupiter-Liar
Copy link

I'm seeing some problems with this one. Some significant problems:

  1. Small problem: If I import a file and then attempt to import it again, the input value does not change, and so the import event is not triggered.

  2. Medium problem: If I import a file, the rules get added to the active page, regardless of whether they were duplicates. And until I refresh, things like saving will fail, citing a "Mismatch in lengths".

  3. BIG problem: Importing is prone to duplication and overwriting.

For testing purposes, I made some short lists. One has 15 rules. One has 6 rules, and is a pruned-down version of the other. If I import them both, I get 21 rules. Even when I refresh the page, the duplicates are still there.

But it gets worse. If I import again, and I'm not sure exactly how it's happening, but the rules I import can overwrite the rules that are already there. This could result in a loss of user rules. And personally, if that happened to me, it would ruin my day.

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

Successfully merging this pull request may close these issues.

Builtin Example Doesn't Work [Bug] Existing but edited redirects imported as new instead of being updated
6 participants