cvzi / genius-lyrics-userscript

A userscript library to show lyrics from genius.com on other websites
GNU General Public License v3.0
14 stars 5 forks source link

Coding regarding 4 themes #18

Closed cyfung1031 closed 1 year ago

cyfung1031 commented 1 year ago

I have noticed that there are similar but different implementations in 4 themes. (like clear / remove / hide the elements)

As the recent scrollIntoView change is in "Genius React". How about other themes? deprecated? Which theme you are using in Spotify?

cyfung1031 commented 1 year ago

I try to omit the redundant coding. I created a "themeCommon" to store all common functions like removal ads to let all theme use the same set of functions. I am not sure the different implementations are just because you forgot to update all themes or intent to make coding with same function names but different implementations

cvzi commented 1 year ago

I am not sure if the old ("Default") Genius theme is still necessary. I think there used to be a button at https://genius.com/ to get the old page design back, but I don't see it anymore, so maybe the completely removed the old page design now and only the "React" theme is needed now.

The white theme and the "Spotify" theme always looked like this. I never really use them myself, but they look okay on Spotify.

cvzi commented 1 year ago

I probably forgot about the implementations, I mainly tried to maintain the "Genius" theme.

cvzi commented 1 year ago

The "Click here to go to the old song page" is still there when I login to genius.com. When I click it, it redirects with ?bagon=1. If I go to https://genius.com/Fazerdaze-come-apart-lyrics?bagon=1 I see the old page design, and when I then go to https://genius.com/Fazerdaze-come-apart-lyrics I see the new page design again. So it doesn't seem to store this in the session.

I guess that means the script will never get the old page design anymore, so the "Genius (Default)" theme could be removed, and the logic that decides whether to use Default or React theme could be removed as well.

cyfung1031 commented 1 year ago

I am going to add some styling to the iframe content so that all other unnecessary information will be hidden. (such as footer)

Screen Shot 2022-12-16 at 8 58 10

Showing this header information and lyrics only.

You want me to add to this genius lyrics as well or just youtube genius lyrics as a new feature?

cvzi commented 1 year ago

Add it to the "library". If I don't like it, I can always create a new theme for Spotify

cyfung1031 commented 1 year ago

Add it to the "library". If I don't like it, I can always create a new theme for Spotify

Screen Shot 2022-12-17 at 21 31 25

Why to show these two icons in the lyrics list? Can Remove?

For the "COMPLETE" state, any example for lyrics with "not complete" state? If all lyrics are complete, why have to show it?

cyfung1031 commented 1 year ago

@cvzi seems the clean white and Spotify theme not working.

As they are not working and no one reported, I guess no one use these two themes. You might consider to remove them completely so that no need to maintain them.

Screen Shot 2022-12-17 at 21 51 10 Screen Shot 2022-12-17 at 21 51 20
cvzi commented 1 year ago
Screen Shot 2022-12-17 at 21 31 25

Why to show these two icons in the lyrics list? Can Remove?

Yes you can remove/change them. I copied them from the Spotify script, where I made them to look similar to the design of the Spotify music search results (Spotify design has changed since then, so they don't look like anything anymore...).

For the "COMPLETE" state, any example for lyrics with "not complete" state? If all lyrics are complete, why have to show it?

It used to be common with very new songs, but I can't find any now. So I guess you can remove it as well.

If you want to redesign the whole search/list thing, go ahead.

cvzi commented 1 year ago

@cvzi seems the clean white and Spotify theme not working.

I don't get the same error. They both work for me, no errors 🤯

cyfung1031 commented 1 year ago

@cvzi seems the clean white and Spotify theme not working.

I don't get the same error. They both work for me, no errors 🤯

As per today script updated, could you please check whether any error generated if clean white / spotify theme is selected? Coz I cannot apply these two themes to test. I have implemented some HTML response text minimization in https://github.com/cvzi/genius-lyrics-userscript/pull/32. For Genius Default theme it is fine, but I am not sure about the other two themes. Hope that it works. If there is any bug raised due to this text minimization, please let me know.

To minimize the text is because I noticed that the cache for the entire lyrics result page is too huge. remove some tags and pre-store some resources like svg could help a lot reduction.

cyfung1031 commented 1 year ago
Screen Shot 2022-12-17 at 21 31 25

Why to show these two icons in the lyrics list? Can Remove?

Yes you can remove/change them. I copied them from the Spotify script, where I made them to look similar to the design of the Spotify music search results (Spotify design has changed since then, so they don't look like anything anymore...).

For the "COMPLETE" state, any example for lyrics with "not complete" state? If all lyrics are complete, why have to show it?

It used to be common with very new songs, but I can't find any now. So I guess you can remove it as well.

If you want to redesign the whole search/list thing, go ahead.

The YouTube Genius Lyrics is now redesigned as per https://github.com/cvzi/Youtube-Genius-Lyrics-userscript/pull/25. I changed some CSS so the update would be more automatic via CSS. You might refer some coding for the Spotify plugin as well. The old coding do a lot xxx.style.xxx = xxx which is hard for customization. For the spinner location, just use flexbox to make it into center / middle without any resize event handler.

For the spinner and spinnerHolder, I make them as spinnerDOM object. It allows much more flexibility for the plugin scripts to do the customization in DOM. The spinnerHolder in YouTube Genius Lyrics is no longer directly appended to document.body (appended to the container of iframe instead)

As now the theme has been successfully redesigned, I would not need to make frequent pull request anymore unless I found some features worth to change or add.

PS. I kept the "COMPLETE" but removed the pageviews since most lyrics do not have this stats. Also it helps reduction of the hit cache as stats is removed.

Regarding the hit cache minimization, default is no minimization. It depends on how many information required by the plugin. For the YouTube Genius Lyrics, it do not need image url, stats, etc. So that all these info are removed and leaving only the titles and names.

cyfung1031 commented 1 year ago

Add it to the "library". If I don't like it, I can always create a new theme for Spotify

I make it as option and the default is disabled. For YouTube Genius Lyrics, now all the irrelevant information inside iframe are removed. Only lyrics and the header information regarding the song / lyrics remain. Due to this minimization, the lyrics page cache can be further reduced by using style replacement (style substitute)

fetch html response text -> minimize the text response -> basic checking the style written in the text response [refer as genius.option.enableStyleSubstitution = true] -> replace them by the pre-stored CSS rules (simplified) [refer as defaultCSS(...)] -> add the customized CSS [refer as contentStyling(...)] with the customized CSS properties [refer as genius.styleProps]

genius.styleProps is generated by the main window which reads the page styling using getComputedStyle(...) therefore the iframe content can change the background colors and text colors accordingly. However it make the "theme selection" not working anymore. As it only do the simplest styling for the content.

The good things is, enableStyleSubstitution can be turned on so that the cache can be further reduced. From my experience, originally genius.option.trimHTMLReponseText = true by default can already reduce the cache to 50% of the original size (thanks to the pre-stored SVGs) if enableStyleSubstitution enabled with contentStyling, it can further reduced to 15%~25% of the original size.

cvzi commented 1 year ago

Thanks for the explanation!

I will take a closer look when I have some more time.

I added a temporary fix for the "White" and "Spotify" theme for Spotify, so it at least shows the lyrics instead of the error.

cvzi commented 1 year ago

As now the theme has been successfully redesigned, I would not need to make frequent pull request anymore unless I found some features worth to change or add.

In the next few days I will give you access to the two repositories on Github in case you want to make bugfixes without me (Greasyfork is set to update automatically from the Github repositories). I just have to find out how to do this on github

cyfung1031 commented 1 year ago

Possible to have desktop_react and desktop_react_atf for WithPrimis as per https://github.com/cvzi/genius-lyrics-userscript/pull/37

Screen Shot 2022-12-21 at 13 24 52

This causes the CSS not compatible with each other... to be resolved.

Reproduction by cookie-manager.user.js cook._genius_ab_test_cohort = 0 for desktop_react_atf cook._genius_ab_test_cohort = null for desktop_react

cyfung1031 commented 1 year ago

for the existing four three themes, can you use pure css rules to replace them completely? I noticed that there are many onload functions. I think some onload functions are still needed like base url however these all onload functions shall be common for all themes. not theme specific. I want to reduce the duplications for three schemes with combine() and scripts()

actually if you set the color stuffs as custom CSS properties, it is more easy for customization.

for example,

 html[theme-selected="white"]{
     --egl-color: white;
     --egl-background: black;
}
 html[theme-selected="black"]{
     --egl-color: black;
     --egl-background: white;
}
 span{
     color: var(--egl-color, '#fff');
}
 span > a{
     color: inherit;
}

for the no use stuff, just display: none !important; no rendering or layout calculation for display: none

cvzi commented 1 year ago

I'll work on the two themes now. I'll try to reuse the stuff from the Genius Theme and move to themeCommon. I think most of the onload stuff in the White and Spotify theme are leftover from the old genius page design, so I can remove it anyway.

cyfung1031 commented 1 year ago

I'll work on the two themes now. I'll try to reuse the stuff from the Genius Theme and move to themeCommon. I think most of the onload stuff in the White and Spotify theme are leftover from the old genius page design, so I can remove it anyway.

I cannot find the discussion section in this repo. I have typed what I thought in the wiki page.

https://github.com/cvzi/genius-lyrics-userscript/wiki/Works-to-be-continued-for-Styling

This is the goal right now. I will also review the issues related with "WithPrimis" as well.

cyfung1031 commented 1 year ago

I'll work on the two themes now. I'll try to reuse the stuff from the Genius Theme and move to themeCommon. I think most of the onload stuff in the White and Spotify theme are leftover from the old genius page design, so I can remove it anyway.

If the styling is more by this Userscript instead of relying the genius native parts, there will be less concern regarding removal of the default CSS from the html response text.

For me, I just need the lyrics, so everything else could be removed. just leaving the basic info header.. therefore I can do it aggressively by hiding all other stuffs. Not sure what is your original idea of the styling...

cyfung1031 commented 1 year ago

As now the theme has been successfully redesigned, I would not need to make frequent pull request anymore unless I found some features worth to change or add.

In the next few days I will give you access to the two repositories on Github in case you want to make bugfixes without me (Greasyfork is set to update automatically from the Github repositories). I just have to find out how to do this on github

Could you please check that whether the webhooks for these two repos are set or not? https://greasyfork.org/en/users/webhook-info https://github.com/cvzi/genius-lyrics-userscript/settings/hooks https://github.com/cvzi/Youtube-Genius-Lyrics-userscript/settings/hooks

I feel like there are few hours delay.....

cvzi commented 1 year ago

Sorry, had to do something else first.

I'll remove everything that I know is not used or is duplicate now.

The other two themes didn't have much styling, they are just text and everything else removed. And the background colors matching Spotify or for the White theme matching Youtube in Light Mode.

As long as that div container that contains the lyrics is still there, they should not need much maintenance, once they work again.

~Webhooks aren't set, it is set to sync from URL. I'll change it.~ Done

cvzi commented 1 year ago

I removed everything that was not used, renamed some functions (Remove the trailing numbers from name) and moved the code that handles the annotations to commonTheme.

cyfung1031 commented 1 year ago

I removed everything that was not used, renamed some functions (Remove the trailing numbers from name) and moved the code that handles the annotations to commonTheme.

I can switch to clean white and Spotify theme now. Both themes are just with lyrics text and without any other information right? and the difference between them is text color and background color only?

cvzi commented 1 year ago

Yes. The Spotify theme used to have a few more adaptions to make it look similar to Spotify, but I don't think I will add that back.