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

[PM-3316] Feature addition - Toggle Hardware Acceleration [Desktop] #5968

Merged
merged 9 commits into from
Mar 21, 2024
Merged

[PM-3316] Feature addition - Toggle Hardware Acceleration [Desktop] #5968

merged 9 commits into from
Mar 21, 2024

Conversation

prithvi2k2
Copy link
Contributor

Type of change

- [ ] Bug fix
- [x] New feature development
- [ ] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

Added a toggle for disabling/enabling hardware acceleration (enabled by default) on Desktop client - Addresses Issue #2615

Code changes

  • apps/desktop/src/

    • app/accounts/settings.component.html: Added checkbox-type form group for toggling feature
    • app/accounts/settings.component.ts: Updates state of checkbox for the feature on change
    • locales/en/messages.json: Added title and description for the feature setting
    • main.ts: On (re)start of application, disables hardware acceleration if turned off
  • libs/common/src/platform/

    • services/state.service.ts: Getter/setter of boolean enableHardwareAcceleration
    • abstractions/state.service.ts: Getter/setter declarations
    • models/domain/global-state.ts: Global state boolean variable declaration

Discussion at PR #2896 by @evelez7 and @Hinton helped me immensely in quickly understanding the requirements.

Screenshots

Screenshot 2023-08-05 135937

@prithvi2k2 prithvi2k2 requested review from a team as code owners August 5, 2023 08:34
@CLAassistant
Copy link

CLAassistant commented Aug 5, 2023

CLA assistant check
All committers have signed the CLA.

@bitwarden-bot
Copy link

Thank you for your contribution! We've added this to our internal Community PR board for review.
ID: PM-3316

@bitwarden-bot bitwarden-bot changed the title Feature addition - Toggle Hardware Acceleration [Desktop] [PM-3316] Feature addition - Toggle Hardware Acceleration [Desktop] Aug 5, 2023
@bitwarden-bot
Copy link

bitwarden-bot commented Aug 5, 2023

Logo
Checkmarx One – Scan Summary & Details80679e2f-2047-4d5e-a054-42dcc362fc51

No New Or Fixed Issues Found

@prithvi2k2
Copy link
Contributor Author

Requesting @djsmith85 and @Hinton for PR review

Copy link
Contributor Author

@prithvi2k2 prithvi2k2 left a comment

Choose a reason for hiding this comment

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

Reviewed

@prithvi2k2
Copy link
Contributor Author

What does "EnforceLabel : Expected -- Waiting for status to be reported" even mean? It's been waiting since I've opened the PR... Is there something I need to do to get through it?

@djsmith85
Copy link
Contributor

@prithvi2k2: This is just job of the build-pipeline (checking for specific labels). Everything required by you has been checked and your contribution has been passed on to review by the product team.

@prithvi2k2
Copy link
Contributor Author

Thank you for the confirmation @djsmith85

@prithvi2k2
Copy link
Contributor Author

Hello @djsmith85 , it's been a long wait for review... I don't mind if it takes even longer as I know how busy the team can get involved in other priority tasks, but is there any chance this PR can be considered for hacktoberfest by adding a hacktoberfest-accepted label? It'll be helpful to me

@djsmith85
Copy link
Contributor

@prithvi2k2 I have passed the feedback onto the product team, which is reviewing your contribution.

@djsmith85 djsmith85 removed the request for review from a team December 7, 2023 12:39
@trmartin4
Copy link
Member

Hello @prithvi2k2! We are interested in pursuing adding this feature to our desktop application. Is this something you would still be willing to work with us at this time to include?

@prithvi2k2
Copy link
Contributor Author

Hello @trmartin4 , Please suggest any necessary changes if needed... I'm still willing to work on it and get it merged to be a part of Bitwarden Developer Community

@prithvi2k2
Copy link
Contributor Author

It's been several months since this pull request was submitted, and I've updated my forked branch with the latest changes from the Bitwarden main branch. I would highly appreciate it if you could take a moment to review my updates

@trmartin4
Copy link
Member

Thank you! We're going to have a review from our design team to make sure there are no adjustments to the UI necessary, and then we'll progress the change through our internal PR review and QA.

@Hinton
Copy link
Member

Hinton commented Mar 18, 2024

Hi @prithvi2k2,

I refactored the state to use the new state provider framework since we've deprecated the old state services and don't want to write migrations for new code.

Copy link
Member

@justindbaur justindbaur left a comment

Choose a reason for hiding this comment

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

All these review comments are part of the changes @Hinton made.

apps/desktop/src/services/desktop-settings.service.ts Outdated Show resolved Hide resolved
apps/desktop/src/services/desktop-settings.service.ts Outdated Show resolved Hide resolved
libs/common/src/platform/state/state-definitions.ts Outdated Show resolved Hide resolved
@Hinton Hinton requested a review from justindbaur March 18, 2024 20:51
justindbaur
justindbaur previously approved these changes Mar 18, 2024
@Hinton
Copy link
Member

Hinton commented Mar 19, 2024

This was handed off to QA yesterday and is currently scheduled to be tested in the near future.

# Conflicts:
#	apps/desktop/src/app/accounts/settings.component.ts
@Hinton Hinton merged commit cd5dc09 into bitwarden:main Mar 21, 2024
47 of 55 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants