overdodactyl / ShadowFox

A universal dark theme for Firefox
https://overdodactyl.github.io/ShadowFox/
MIT License
1.32k stars 58 forks source link

Combine UUID finder/adder #84

Closed Shourai closed 6 years ago

Shourai commented 6 years ago

The UUID finder is a great script, thanks for that! I was wondering though if it isn't better to combine the UUID adder and finder.

Is the intermediate step of saving the UUIDs to a .txt file necessary? From a perspective of someone installing ShadowFox, I would think that person would want everything styled to this theme.

echo "${styled[$id]}_UUID=$uuid" >> 'ShadowFox_customization/internal_UUIDs.txt'

in internal_UUID_finder.sh changed to

sed -i '' "s/${styled[$id]}_UUID/$uuid/" "/userContent-files/webextension-tweaks/${styled[$id]}.css"

does exactly what add_UUID.sh does without the need of an intermediate .txt file.

What do you think about this?

overdodactyl commented 6 years ago

I think it's certainly worth re-considering the structure and function of some of the scripts in the repo. I had kinda been adding them on either an as-needed basis or when I got around to having the time to incorporate something new..so there's probably a better way to combine some of them.

That being said, I do still feel like there's some benefit to using internal_UUIDs.txt, even though it's slightly less efficient, due to the extra layer of customization it provides.

You're probably right in that most people would generally want all or nothing in terms of the extension styling, but I could see there being some exceptions as well. As an example, I haven't yet added support for the color-blind friendly color scheme uMatrix offers. I would hate for someone who needed (or simply prefers) this version to have to either have no extensions styled or to manually find and remove the UUID in userContent.css every time they update because it's automatically inserted based on the extension being downloaded. While the uM example could quickly be fixed by me adding some styling, I think it's also a symptom of a larger problem: extensions are a lot more dynamic and often require more detailed focus than other parts of the repository. The extensions I personally use will likely always be kept up-to-date and fully usable, but I can't necessarily promise that will be the case for all extensions in the repo. Many of the extensions there are currently styles for were made based on a request. If I don't receive help maintaining extensions from the people who use them, or at the very least notified when an update causes issues with my current styling, I could see where some usability problems would arise. Because of this, I think people should ideally have a really easy way to opt-in/out of having a specific extension altered by ShadowFox.

The other issue we have at play here is the various ways people can utilize the repository. Some people are making use of userChrome/Content_imports, while most are just using userChrome/Content.css.

add_UUIDs.sh was made for those using userChrome/Content_imports as it inserts the UUID in the corresponding file within the userContent-files/webextension-tweaks directory (a directory many users don't have).

As such, having them separated makes internal_UUID_finder.sh useful for everyone. For people just using the 2 files, it can be called by the updater script and then the UUIDs will be inserted into userContent.css. For those that are using the import files, it can be called to generate the UUIDs, and then add_UUID.sh can be used.

All these problems could admittedly probably be addressed by creating some conditionals in the script to determine what actions to perform..I would just need to give a little closer consideration on what exactly to do.

Any thoughts?

Shourai commented 6 years ago

Good points, I am personally using the @imports version of shadowfox and didn't consider the difference between this version and the non-@import version.

I thought about instead of removing the extension UUIDs inside internal_UUIDs.txt why not let the user remove the extensions from the internal_UUID_finder.sh. That would probably lead to the same effect, unless you're also planning to have that script update itself.

The idea with the conditionals would be a good way to merge the two files, you could ask the user to either "find UUIDS" or "find and apply UUIDs". The cleanest way, seen from the user, would be having a list of a few choices when running the combined add/find/remove script.

1) Find UUIDs
2) Apply UUIDs
3) Find and apply UUIDs
4) Remove UUIDs

However this would create a fairly large file which, to do it right, needs good structuring.

I have an honest question, what is the difference between the @import version and the regular version? Why would one user choose the regular version over the @import version? The @import version might be a little slower but is it significant (I don't know how to measure it)? All the imports are done locally so we don't have http requests to worry about which would indeed increase loading times.

The updater script (if I read it correctly) backups the old user{Chrome,Content}.css and downloads the latest version. The changes the user made are not honored in those files, as they come from *_customization.css.

Just an idea I had was, maybe instead of the _customization.css use sed to extract the user customization from the backed up .css files. Add an indicator line like you did for coloroverrides to make it an easy regex lookup. This would work well with the @imports version since it's easy to see what ones own additions are. It would also cut down on the amount of files to update.

Just some brainstorming, what do you think?

overdodactyl commented 6 years ago

I definitely like the idea of joining the scripts and giving users the various options you mentioned. Also would agree it would be a good idea to give people the option to remove UUIDs from the script instead of having to manually do it via internal_UUIDs.txt. I'm still a little hesitant to get rid of the file altogether though, as it seems like a good way to preserve "preferences" and simplify the differences caused by the imports vs normal versions...but perhaps I'm wrong about this. As a less important point (as it likely only effects me and a few others), removing the use of internal_UUIDs.txt would complicate the remove_UUIDs.sh script as well. Any time I update ShadowFox, I run this so the UUIDs of my extensions don't get pushed to the repo.

For a while, I think it was generally deemed "best practice" to avoid using @imports when possible, but to my knowledge most of the problems with them have either been solved (such as styles not being loaded in the right order when you have multiple @import statements in a row) or aren't relevant to this use case (such as the extra HTTP requests or parallel loading <- also may have been fixed recently). When I first started the project only the imports version available and I didn't really have the intention of creating a concatenated version. The need for that came from a Firefox bug/quirk in Linux that prevents the usage of @imports in userContent.css. Because of that, the only way to get things working was to squash everything into a single file.

For some use cases though, I do think it really simplifies things though. For people who don't really need or want a high level of customization, I would say it makes things simpler to just deal with two files instead of the entire repository. Definitely makes updating easier as well for users who don't want to deal with a GitHub fork or git (to my knowledge, there's no way to download an entire repository or directory from command line/script without a tool like git installed).

The changes the user made are not honored in those files, as they come from *_customization.css.

The idea for those not using the imports version would be to never really touch userChrome/Content.css. Any alterations they want to make would go in *_customization.css. These files get appended to the very end of userChrome/Content.css upon updating. As such, if there's any duplicate rules in ShadowFox and the customization files, the customization files take precedence. Obviously if they were unique rules, they too would go into effect. I originally thought this would be the easiest way for users to track their own changes as it's in a separate file (users who aren't super familiar with css wouldn't have to deal with/understand the already fairly long user*.css files) and it can be maintained separately from the repository (anyone who had changes prior to ShadowFox can essentially just make their old user*.css file the *_customization.css ones.

Essentially instead of using the indicators, I opted to just attach everything to the end of the file. I had to use indicators for the colors because they have to go in a very specific location (i.e. after they are defined by ShadowFox, but before they are used anywhere). I think the biggest downside to adding indicators to each file would be that users would have to have a better understanding of both this project and css in general..it would be harder to just copy/paste tweaks they find elsewhere and place them in the relevant spot.

I wouldn't say I'm stuck in my ways though...so I'm definitely not opposed to changing things. Thoughts?

CaptaPraelium commented 6 years ago

Just my anecdotal thoughts on this - I've never bothered to look up the UUIDs, it's too complex and since I rarely look at those, it's not worth my effort. But, it would be nice to have them themed! If the UUIDs were found and the addons themed by some magic I'd really like that.

overdodactyl commented 6 years ago

@CaptaPraelium, that functionality has been added to the mac and linux updater scripts, but has not yet made its way to the Windows one yet unfortunately