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

Custom Prompt for changing options #243

Merged
merged 32 commits into from
Oct 24, 2021
Merged

Conversation

Araxeus
Copy link
Collaborator

@Araxeus Araxeus commented Apr 28, 2021

Implements Custom Electron Prompt

I pretty much made this package to easily set different options in youtube-music

There are currently 4 available custom types:

  • Input: Just a general text input field.

  • Keybind: Allow capturing single or multiple keybinds (Electron Accelerator format)

  • Counter: Similar to input, but accept only numerical values and has [+] and [-] Buttons for ease of use

    • Used for setting custom volume steps in precise-volume
  • Select: Create a dropdown select menu, doesn't seems to have use in this app since menu radio buttons exist


etc

  • Creating a prompt counts as a browser-window-created event, So:
    • using win.once("did-finish-load") instead of win.on in index.js fixes listener registering on every window created
    • require("electron-debug")({showDevTools: false}) remove devTools on new prompt in dev mode
Prompt with customTitlebar needs testing on macOS, I disabled it for that platform for now
--Click to view code-

youtube-music/menu.js

Lines 326 to 335 in 6d44a57

//TODO: custom bar on prompt need testing on macOS
if(!is.macOS()) {
Object.assign(options, {
frame: false,
customScript: path.join(__dirname, "providers", "prompt", "customTitlebar.js"),
enableRemoteModule: true,
height: 200,
width: 450,
});
}

@Araxeus Araxeus marked this pull request as draft May 2, 2021 21:20
@Araxeus Araxeus marked this pull request as ready for review May 4, 2021 21:43
@Araxeus Araxeus changed the title Implement custom-electron-prompt Custom Prompt for changing options May 4, 2021
Copy link
Owner

@th-ch th-ch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a few comments after a first review, looks promising 🤩
nit: when the prompt window appears, the background is white, and it only gets black in a second time (and the change is noticeable), is it expected?

menu.js Outdated Show resolved Hide resolved
menu.js Outdated Show resolved Hide resolved
menu.js Outdated Show resolved Hide resolved
menu.js Outdated
let options = {
title: 'Set Proxy',
label: 'Enter Proxy Address: (leave empty to disable)',
value: config.get("options.proxy") || example,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Idea of feature for the prompt plugin: the ability to have a placeholder (would be better suited for example instead of putting it as value and checking later that value !== example)

Copy link
Collaborator Author

@Araxeus Araxeus May 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is already a placeholder which is the value field

instead of putting it as value and checking later that value !== example

Are you talking about implementing this exact logic in custom-electron-prompt ?

I could make an boolean option placeholder which will mean "return null if output=options.value"
but it seems kinda unnecessary..

I think it might be best to let whoever implements the prompt decide what to do with the output, since there could be many different intentions when using the prompt,

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, idea was to add a implement the placeholder logic/field in custom-electron-prompt! (main pro of a placeholder is to avoid having a fake value to remove when filling the field, contrary to a default value - here, it feels we have a placeholder used a default value 🙃)
Not a big deal though, does not block merging - just an improvement idea for the custom-prompt package :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

355f611 changed the input field "placeholder" attribute to the previous example as you suggested
(didn't need to change custom-electron-prompt itself)

I'm not sure its better though, since you can't copy paste or edit that text, and it disappears when typing. which can makes it harder for people that don't know the format

menu.js Outdated Show resolved Hide resolved
config/defaults.js Show resolved Hide resolved
plugins/precise-volume/menu.js Outdated Show resolved Hide resolved
Copy link
Owner

@th-ch th-ch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the prompt-custom-titlebar, looks ok to keep it all the time (but disabled for MacOS to avoid issues) but it should be moved to the providers folder in that case! (so that no in-app-menu code is executed if the plugin is disabled)

console.warn("Invalid action", action);
return;
/** Update options to new format if they are still an array (old format) */
function updateOptions(options) {
Copy link
Owner

@th-ch th-ch May 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a one-shot operation that can be done with a store migration (there are examples in in config/store.js)

Copy link
Collaborator Author

@Araxeus Araxeus May 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved the code to store migration in 002081b
Is it alright this way? should the version be higher?

(prompt-custom-titlebar.js was already moved to providers folder before 😝)

menu.js Outdated
let options = {
title: 'Set Proxy',
label: 'Enter Proxy Address: (leave empty to disable)',
value: config.get("options.proxy") || example,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, idea was to add a implement the placeholder logic/field in custom-electron-prompt! (main pro of a placeholder is to avoid having a fake value to remove when filling the field, contrary to a default value - here, it feels we have a placeholder used a default value 🙃)
Not a big deal though, does not block merging - just an improvement idea for the custom-prompt package :)

Copy link
Owner

@th-ch th-ch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks for the contribution! Just need to fix the merge conflict and it should be good to merge! ✅

@th-ch
Copy link
Owner

th-ch commented Jul 19, 2021

I am getting TypeError: Cannot set property 'previous' of undefined in shortcuts/menu.js:45 when trying to register global song controls, not sure if it's expected? (it seems the migration does not add the global/local keys in config when they are not present)

@th-ch th-ch mentioned this pull request Jul 19, 2021
@Araxeus
Copy link
Collaborator Author

Araxeus commented Jul 20, 2021

I am getting TypeError: Cannot set property 'previous' of undefined in shortcuts/menu.js:45 when trying to register global song controls, not sure if it's expected? (it seems the migration does not add the global/local keys in config when they are not present)

Indeed the object needs to be created before setting its properties 😅
should be fixed now

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