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

global bypass toggle #21

Merged
merged 5 commits into from
Jul 15, 2024
Merged

global bypass toggle #21

merged 5 commits into from
Jul 15, 2024

Conversation

remisiki
Copy link
Contributor

This pr adds a toggle button at the top of the menu that can enable/disable global bypass option, which can be useful when users want to turn on/off the effects from the top panel.

Copy link
Owner

@ulville ulville left a comment

Choose a reason for hiding this comment

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

Thanks Good idea!

There are some issues that needs to be resolved before I can merge this though.

Functional Issues

  1. You should do // Get global bypass section before calling the _buildMenu function in the _refreshMenu. With its current state if you change the global bypass from the easyeffects gui, it won't show the correct status on the toggle until you open the menu for a second time.

    Right above the // Try to get Last used presets line seems like a suitable place.

    Don't be confused by the fact that we call the _buildMenu twice. The first one is for the menu to open with last calculated values while we run our async cli operations. (Otherwise it will flash absurdly when we update the menu with the new values). You should place get global bypass logic after the first call but before the second.

  2. // Get global bypass section probably should be in a try-catch block. Otherwise a potential error will go un-handled and without any log/notification.

  3. Take a look at the // Arrange scrollbar policies section. We need to calculate the total height of other items we add to determine whether we need to enable scrollbar. You'll need to bring toggleBypassItem into that equation.

Cosmetic Issues

  1. Not a necessity but consider moving the toggle to the bottom, between the _separatorItem and the _easyEffectsActivatorItem. I think it looks better if they are grouped together instead of being scattered all over the place.
  2. Make the text Title Case. Capitalize the first letters of every word. And just "Global Bypass" is enough. No need for "Enable" because it's already obvious.
  3. Use toggleBypassItem.setOrnament(PopupMenu.Ornament.NONE); So the text can be aligned with other items.

@remisiki remisiki requested a review from ulville July 14, 2024 00:03
@ulville
Copy link
Owner

ulville commented Jul 14, 2024

Great! Thanks @remisiki this all looks good to me. Tested locally and I think it's good to go, except the lack of linting.

Can you please use eslint to lint it with my eslint config (which is basically the de-facto standard for gnome extensions but may be a bit outdated) so I don't need to clutter the commit history with my linter commit afterwards.

Create a package.json file in the root of the project:

{
  "dependencies": {
    "eslint": "^8"
  },
  "devDependencies": {
    "eslint-plugin-jsdoc": "^46.2.3"
  }
}

then run:

npm install

then finally run:

npx eslint --fix -c .eslint.yml extension.js

This occasion reminded me that I should include my package.json in the git at some point or use Github's CI to force it and probably I should go ahead and update my eslint config according to gjs.guide if it's changed after I took it.

Copy link
Owner

@ulville ulville left a comment

Choose a reason for hiding this comment

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

Thanks

@ulville ulville merged commit 711d816 into ulville:master Jul 15, 2024
This pull request was closed.
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.

2 participants