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.

Sign up for GitHub

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

Jump to bottom

feat: add zoom config on UI and fix zoom-related issues #853

Merged
merged 6 commits into from
Oct 30, 2024
Merged

feat: add zoom config on UI and fix zoom-related issues #853

merged 6 commits into from
Oct 30, 2024

Conversation

ShGKme
Copy link
Contributor

@ShGKme ShGKme commented Oct 28, 2024

☑️ Resolves

  • Fixes: Add configuration option/screen for zoom #793
  • Adds zoomFactor config to application config
  • To handle zoom change from different sources, adds listening to config changes feature
  • Adds zoom config to the UI
  • Considers zoom in title bar height and initial/min window size

Implementations notes

Solution complexity

Zoom can be changes with user settings

  • Via new user settings
  • Via CTRL + 🖱️/Touchpad gesture - implemented by handling the zoom-changed event
  • Via CTRL + +/- shortcut - included to the standard corresponding MenuItems and cannot be handled

Because the last option is not possible to handle in Electron at all, and there is no generic zoom changed event, the solution is a bit complex.

Zoom step

On each step zoom can be changed:

  1. Adding a percent, e.g. +20%
    • Scale on level N = 1 + 0.2 * N
    • 20% - 40% - 60% ... 100% - 120% ... 300% - 320%
    • Simple and understandable for a user
    • Non liner. Change 20% -> 40% is dramatic x2 while 300% - 320% slightly visible
  2. Applying a percent, e.g. x20%:
    • Scale on level N = 1.2 ** N
    • 100% - 120% - 144% - 172.8%
    • Uniform increase
    • Default Electron behavior
    • Hard to understand by user and not beautiful numbers
  3. Having a specific pre-defined list of values
    • More complex implementation
    • Beautiful and yet uniform

Option 2 was chosen.

🖼️ Screenshots

Settings

settings

Title bar height change (pay attention on the window buttons)

🏚️ Before 🏡 After
before after

Windows on 200%

🏚️ Before 🏡 After
image image
image image
image image
image image

@ShGKme ShGKme added enhancement New feature or request 3. to review labels Oct 28, 2024
@ShGKme ShGKme self-assigned this Oct 28, 2024
@ShGKme ShGKme changed the title feat: add zoom config on UI and fix zoom-relaetd issues feat: add zoom config on UI and fix zoom-related issues Oct 28, 2024
@ShGKme ShGKme mentioned this pull request Oct 28, 2024
Open
Antreesy
Antreesy approved these changes Oct 29, 2024
View reviewed changes
Copy link
Contributor

@Antreesy Antreesy left a comment

Choose a reason for hiding this comment

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

Tested on Win

src/app/AppConfig.ts Show resolved Hide resolved
src/talk/renderer/Settings/DesktopSettingsSection.vue Outdated
:aria-describedby="descriptionId"
inputmode="number"
:value="zoomFactorPercentage"
@change="zoomFactorPercentage = $event.target.value" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a big deal, but should we handle border values here (NcTextField, NcButton)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is handled by zoomFactorPercentage

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant that it's still possible to set out of range values in UI, they just not kept in config and not reflected in the app window

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, works better with manual input. Maybe same getter/setter could be done for zoomFactor?
2024-10-29_16h43_11

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It has different limits. Will put it to constants

DorraJaouad
DorraJaouad reviewed Oct 29, 2024
View reviewed changes
src/app/zoom.service.ts Outdated

browserWindow.webContents.setZoomFactor(zoom)
// Resize the title bar to match the new zoom level
browserWindow.setTitleBarOverlay({
Copy link

Choose a reason for hiding this comment

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

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

DorraJaouad
DorraJaouad reviewed Oct 29, 2024
View reviewed changes
src/talk/renderer/Settings/components/SettingsSelect.vue Outdated
Comment on lines 44 to 49
<NcSelect v-model="model"
class="settings-select"
:options="options"
:clearable="false"
:searchable="false"
:input-id="inputId" />
Copy link

Choose a reason for hiding this comment

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

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Contributor

Choose a reason for hiding this comment

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

image

@Antreesy
Copy link
Contributor

After hard restart window size will be 100%, regardless of config value

B A
image image

Also label next to NcSelect is overlayed

DorraJaouad
DorraJaouad approved these changes Oct 30, 2024
View reviewed changes
Copy link

@DorraJaouad DorraJaouad left a comment

Choose a reason for hiding this comment

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

Tested ✅ 🦅

@ShGKme
Copy link
Contributor Author

ShGKme commented Oct 30, 2024

Also label next to NcSelect is overlayed

This part I cannot reproduce

@Antreesy
Copy link
Contributor

This part I cannot reproduce

Styles aren't scoped to NcDateTimePicker: nextcloud-libraries/nextcloud-vue#6095
image
cc @GretaD

Antreesy
Antreesy approved these changes Oct 30, 2024
View reviewed changes
Copy link
Contributor

@Antreesy Antreesy left a comment

Choose a reason for hiding this comment

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

So bug is unrelated to this PR, new changes look good

@ShGKme
Copy link
Contributor Author

ShGKme commented Oct 30, 2024

Squashed fixups, rebased onto main

@ShGKme ShGKme enabled auto-merge October 30, 2024 10:56
@ShGKme ShGKme merged commit 3e839d5 into main Oct 30, 2024
10 checks passed
@ShGKme ShGKme deleted the feat/config-zoom branch October 30, 2024 11:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Antreesy Antreesy Antreesy approved these changes

@DorraJaouad DorraJaouad DorraJaouad approved these changes

Assignees

@ShGKme ShGKme

Labels
3. to review enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add configuration option/screen for zoom
3 participants
@ShGKme @Antreesy @DorraJaouad