✨ Fix Nav Buttons, Fix Quick access, Update Electron, and more #1142
Conversation
…ependencies. Update the notification title for windows. Added windows readme for how to setup and run as it is quite hard to setup. Major version bump to 1.15.0.
…s "lost". Add additional logic to handle multiple screens for the miniplayer location.
…t to remove --frozen-lockfile
…into development
… Transport Controls (SMTC). Improved the Windows setup readme file.
…niplayer-location
|
Added forward and back buttons. This is based on my previous branch which had other changes & upgrades. |
|
While this PR looks like an absolute Saving Grace of a Final tihng to ship for a v1.15 before we move over to V2.... This changes a lot of stuff, both good and bit more discussion is needed 😵. From my quick skim:
And probably a few other bits which i've missed from my skim review. 😅 |
|
You can see that the 'Fancy' title bar is actually feature-deficient compared to the System one (there is no title of application in the fancy one so it made sense to me to use the default one): This branch includes all my previous changes that have been in a couple different Pull Requests. I have merged in a couple other things (where you're seeing the
The modals are boxes that don't really matter which title bar we use (at least currently as they look basically the same), so it made sense to use the system default ones. |
Alipoodle
changed the title
There was a problem hiding this comment.
😵 So this is only really the initial review of things, but probably the 90% of the things from this overall.
This is kinda designed to be a discussion as well as some changes.
This was discussed by people as well regarding some of the changes made, so this is not souly my own opinion on this.
| } | ||
| ], | ||
| "variables": { | ||
| "openssl_fips" : "0" |
There was a problem hiding this comment.
Confused as to why this is needed, this seels a little suspious
Could you explain this?
There was a problem hiding this comment.
I don't remember, it was to make it build properly.
main.js
Outdated
| icon: iconDefault, | ||
| modal: false, | ||
| frame: windowConfig.frame, | ||
| titleBarStyle: windowConfig.titleBarStyle, | ||
| titleBarStyle: 'default', |
There was a problem hiding this comment.
Again forcing default titlebar when style is upto user choice.
There was a problem hiding this comment.
But we're forcing user choice already to the 'nice' style which is deficient in what it displays - doesn't display the app title. What does it give you over the default system styling (which you can change from)?
There was a problem hiding this comment.
Doesn't display the app title
This is indeed a minor thing to be missing, but given that this was something from the start of the project and how the application has been show.
To my knowledge there was works to get this to likely be themeable in CSS so that it could match your theme / album art. (This was present in the insperation fo this from GPMDP)
There was a problem hiding this comment.
So why show the 'Nice' theme as a default if it:
a) Doesn't show the app title at the top (like the system one does)
b) Is not the Electron default title bar which should ideally be more compatible/same across platforms
There was a problem hiding this comment.
I'm not backing down from this.
When you show off a product you show it off generally with all the bells and whistles to make it visually look good.
The user obviously can go through the settings (And generally considered all users will as there's so many other additions which YTM Desktop offers in features)
That's when they can decide if they want to go back to the System title bar or not.
Changes like this are the sort of things which are discussed by the community and not an individual, before being actioned.
Ultimately this is going to be the last opportunity to get a v1 release before we finalise up the final bits we want for v2 and ship that and v1 is no longer supported at all (given how much of a mess code base is, building not working, how out of date most dependencies are). V2 has been a complete re-code for modern Electron with far better security measures put in place for it to work with modern practices.
Ultimately there's some bigger changes coming in with how the title bar is managed in v2 using some of Electron's new features for "overlaying" over existing controls to provide a Customisable Native Taskbar (Good example inside of VS Code)
Example on MacOS we may not even have a top bar at all and the "traffic lights" will be embedded into the YT top bar.
src/utils/defaultSettings.js
Outdated
| const { app } = require('electron') | ||
|
|
||
| if (isLinux()) settingsProvider.setInitialValue('titlebar-type', 'system') | ||
| if (isLinux() || isWindows()) | ||
| settingsProvider.setInitialValue('titlebar-type', 'system') |
| node-version: 12 | ||
| - run: yarn --frozen-lockfile | ||
| node-version: 18 | ||
| - run: yarn |
There was a problem hiding this comment.
Curious to why we are removing the --forzen-lockfile considering this is intended to be 1-1 to what people are using?
This shouldn't be trying to update things at all.
There was a problem hiding this comment.
Added back. Given this is for the workflow builds, it should still be there; I don't remember why I removed it.
|
With the latest changes |
Alipoodle
added
the
YTMD V1
|
So besides the changes regarding the title bar. And Producing a successful builds (which GitHub Actions is kinda the only hope to make sure that each platform has one...) |
|
By the looks of it, the yarn.lock file is out of sync which is why it failed without the |
|
I'll change the title bars back to configured theme. I'm attempting to upgrade the yarn.lock but having issues with it building the Windows native dependencies... it's such a pain. |
I was messing with updating dependencies but running into issues, so I'm leaving yarn file as is for now. |
|
Currently, only bots and allowed integrations are allow to set Listening/Watching or Streaming (And to my knowledge there might even be Competing now) Doing some v2 work today, but might get some time later to review some of the extra changes. |
main.js
Outdated
| ), | ||
| { | ||
| search: | ||
| 'page=settings/settings&trusted=1&icon=settings&hide=btn-minimize,btn-maximize&title=' + | ||
| __.trans('LABEL_SETTINGS'), | ||
| __.trans('LABEL_SETTINGS') + |
There was a problem hiding this comment.
Duiplicate Search items?
&hide=btn-minimize,btn-maximize
| search: | ||
| 'page=settings/sub/last-fm/last-fm-login&icon=music_note&hide=btn-minimize,btn-maximize&title=Last.FM Login', | ||
| } | ||
| './src/pages/settings/sub/last-fm/last-fm-login.html' |
There was a problem hiding this comment.
These have had the search params removed for it working using the titlebar stuff,
There was a problem hiding this comment.
Page breaks if trying to add the &hide... arguments. The other called out instances have the same behavior as well.
There was a problem hiding this comment.
This is used to pass the additional data to the custom frame and not the embeded frame.
storing these lines to how they were before should be the fix (considering it's done for settings)
| search: | ||
| 'page=editor/editor&icon=color_lens&hide=btn-minimize,btn-maximize', | ||
| } | ||
| path.join(__dirname, './src/pages/editor/editor.html') |
There was a problem hiding this comment.
More search things for Custom titlebar
main.js
Outdated
| 'page=lyrics/lyrics&icon=music_note&hide=btn-minimize,btn-maximize&title=' + | ||
| __.trans('LABEL_LYRICS'), | ||
| } | ||
| path.join(__dirname, './src/pages/lyrics/lyrics.html') |
There was a problem hiding this comment.
More search things for Custom titlebar
| __.trans('LABEL_SETTINGS_DISCORD') + | ||
| '&hide=btn-minimize,btn-maximize', | ||
| } | ||
| 'src/pages/settings/sub/discord/discord_settings.html' |
There was a problem hiding this comment.
More search things for Custom titlebar
| __.trans('SHORTCUT_BUTTONS') + | ||
| '&hide=btn-minimize,btn-maximize', | ||
| } | ||
| 'src/pages/settings/sub/shortcut-buttons/shortcut-buttons-settings.html' |
There was a problem hiding this comment.
More search things for Custom titlebar
| 'LABEL_CHANGELOG' | ||
| )}&page=changelog/changelog&hide=btn-minimize,btn-maximize`, | ||
| } | ||
| path.join(app.getAppPath(), 'src/pages/changelog/changelog.html') |
There was a problem hiding this comment.
More search things for Custom titlebar
yarn.lock
Outdated
There was a problem hiding this comment.
The yarn lock is still broken with the GitHub Actions. >.<
There was a problem hiding this comment.
If you need reproducible dependencies, which is usually the case with the continuous integration systems, you should pass --frozen-lockfile flag.
That's from the Yarn page. So removing that flag will fix it? I find it hard to believe because you'd want these reproducible, but I will put a new commit without that flag.
There was a problem hiding this comment.
The whole point of including the --forzen-lockfile flag is to make sure that the GitHub Actions is running with the intended Packages.
The yarn.lock will check that version, hashes ETC matchup with the package.json, just running yarn to install packages will likely solve this issue.
If you are using npm to install/add packages then this will be the reason for breaking it as the lock files are missaligned as some bits have gone into the node lock and some in the yarn lock.
|
I've done a 'yarn install' for this, and that was the last yarn.lock I committed.
Additionally, there's more changes that still need to be made. I discovered as looking into it was the reason I used the default titlebar style for the settings menus (especially the sub menus) is because it breaks now with the later electron updates. The way we're doing 'require('discord_settings.js')' for example don't work right any longer - it throws an error saying 'require' isn't defined.
I don't want to rewrite everything down to that, so the compromise is the default titlebars for the sub menus.
…________________________________
From: Alipoodle ***@***.***>
Sent: Monday, August 14, 2023 1:04 AM
To: ytmdesktop/ytmdesktop ***@***.***>
Cc: Jason ***@***.***>; Author ***@***.***>
Subject: Re: [ytmdesktop/ytmdesktop] ✨ Fix Nav Buttons, Fix Quick access, Update Electron, and more (PR #1142)
@Alipoodle commented on this pull request.
________________________________
On yarn.lock<#1142 (comment)>:
The whole point of including the --forzen-lockfile flag is to make sure that the GitHub Actions is running with the intended Packages.
The yarn.lock will check that version, hashes ETC matchup with the package.json, just running yarn to install packages will likely solve this issue.
If you are using npm to install/add packages then this will be the reason for breaking it as the lock files are missaligned as some bits have gone into the node lock and some in the yarn lock.
—
Reply to this email directly, view it on GitHub<#1142 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AKNDTHR2TBJI325TCDTPY2TXVHLXRANCNFSM6AAAAAA3AXJBHU>.
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
|
Yeah, no, it doesn't work. I restored that change and it doesn't work.
________________________________
From: Alipoodle ***@***.***>
Sent: Monday, August 14, 2023 1:08 AM
To: ytmdesktop/ytmdesktop ***@***.***>
Cc: Jason ***@***.***>; Author ***@***.***>
Subject: Re: [ytmdesktop/ytmdesktop] ✨ Fix Nav Buttons, Fix Quick access, Update Electron, and more (PR #1142)
@Alipoodle commented on this pull request.
________________________________
In main.js<#1142 (comment)>:
@@ -1429,17 +1495,14 @@ async function createWindow() {
await lastfm.loadFile(
path.join(
__dirname,
- './src/pages/shared/window-buttons/window-buttons.html'
- ),
- {
- search:
- 'page=settings/sub/last-fm/last-fm-login&icon=music_note&hide=btn-minimize,btn-maximize&title=Last.FM Login',
- }
+ './src/pages/settings/sub/last-fm/last-fm-login.html'
This is used to pass the additional data to the custom frame and not the embeded frame.
storing these lines to how they were before should be the fix (considering it's done for settings)
—
Reply to this email directly, view it on GitHub<#1142 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AKNDTHT56SEID4W3OPDQTBLXVHMIDANCNFSM6AAAAAA3AXJBHU>.
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
| @@ -318,11 +322,16 @@ function createContextMenu() { | |||
| } | |||
| } | |||
|
|
|||
| function createTopMiddleContent() { | |||
| function getTrustedHTML(htmlText) { | |||
There was a problem hiding this comment.
Can I ask what the reason for including this function when this is all first party html which is hard coded in?
There was a problem hiding this comment.
Because it was giving an error in the console for not secure content and failing.
|
This is with the following code for loading the lyrics:
await lyrics.loadFile(
path.join(
__dirname,
'./src/pages/shared/window-buttons/window-buttons.html'
),
{
search:
'page=lyrics/lyrics&trusted=1&icon=music_note&hide=btn-minimize&title=' +
__.trans('LABEL_LYRICS'),
}
)
I tried to attach screenshots saying 'require' is not defined.
This is why I changed to just load the windows with the .html files and use the default toolbars. I don't know if this is something new with the new Electron or something.
________________________________
From: Alipoodle ***@***.***>
Sent: Monday, August 14, 2023 1:11 AM
To: ytmdesktop/ytmdesktop ***@***.***>
Cc: Jason ***@***.***>; Author ***@***.***>
Subject: Re: [ytmdesktop/ytmdesktop] ✨ Fix Nav Buttons, Fix Quick access, Update Electron, and more (PR #1142)
@Alipoodle commented on this pull request.
________________________________
In src/utils/injectControls.js<#1142 (comment)>:
@@ -318,11 +322,16 @@ function createContextMenu() {
}
}
…-function createTopMiddleContent() {
+function getTrustedHTML(htmlText) {
Can I ask what the reason for including this function when this is all first party html which is hard coded in?
—
Reply to this email directly, view it on GitHub<#1142 (review)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AKNDTHUCCBC5RUZEQ46Q4H3XVHMSZANCNFSM6AAAAAA3AXJBHU>.
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
Various other improvements. Fixed window positioning saving.
…ith current settings
|
If you want to download my 1.14.2 version to test it out: https://drive.google.com/file/d/1bt594ku2s5h8B4iIVMLD9Tsr7CioJyKA/view?usp=drive_link EDIT FROM YTMDESKTOP: |
…rovider to use my locales and to handle the source not being found.
Added Back and Forward history buttons to the search bar (easiest place that had room) to deal with the Youtube Music UI changes that recently rolled out.