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

Add zoom slider, match official client zoom #370

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

Kaydax
Copy link

@Kaydax Kaydax commented Jan 31, 2024

This code is really bad, so I expect many change request. However its working fine.

This adds a zoom slider to the Vesktop settings page as well as rebinds ctrl+/- to use these zoom controls instead of the built in electron zoom controls.

@@ -470,6 +503,32 @@ export async function createWindows() {
mainWin!.maximize();
}
});

mainWin.on("focus", () => {
Copy link
Member

Choose a reason for hiding this comment

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

this is a really bad way to do this. why not use menu items with accelerators like previously?

Copy link
Author

Choose a reason for hiding this comment

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

Because it refused to work with the menu items after hours of trying. If anyone is able to get it working that way instead that would be super helpful

Copy link
Member

Choose a reason for hiding this comment

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

it works very much the same. did you maybe forget to remove the default zoom handlers? they might take priority over your accelerators

@Vendicated
Copy link
Member

Vendicated commented Feb 18, 2024

i dont think this is very useful. why would anyone use this instead of just using zoom shortcuts?

@sunnniee
Copy link

i dont think this is very useful. why would anyone use this instead of just using zoom shortcuts?

because they simply don't know the zoom shortcuts exist. changing zoom is a reoccurring question in the support channel

@Vendicated
Copy link
Member

people also constantly ask how to change vencord location in vesktop even tho its inside vesktop settings

people are simply stupid. you cant fix that by adding pointless settings

@Kaydax
Copy link
Author

Kaydax commented Feb 18, 2024

i dont think this is very useful. why would anyone use this instead of just using zoom shortcuts?

I explained this in the dev channel. The current zoom shortcut buttons do not match the same zoom scale as the official discord client, thus causing totally different zoom levels between the two. It's more of a OCD thing than anything, but also makes the client feel more like the official one. It also fixes issues with fonts looking off with the way the normal zoom works, as the default electron / chrome zoom have fractional zooming, which makes fonts look blurry on some zoom levels

@DRAGONTOS

This comment was marked as spam.

@TomTheDragon
Copy link

The zoom shortcut does not seem to work on linux (Vesktop and/or official client)
So a slider like the official client has, seems to be the only way to get zoom functionality

@D3SOX
Copy link
Contributor

D3SOX commented Mar 18, 2024

@TomTheDragon I don't why it doesn't work for you but CTRL + 0 / - / + work fine for me on Linux with KDE Plasma

@TomTheDragon
Copy link

@TomTheDragon I don't why it doesn't work for you but CTRL + 0 / - / + work fine for me on Linux with KDE Plasma

I just gave it a try on my other machine and zoom over the shortcut works fine over there.
Both machines run KDE6, so not sure whats the problem then.
But hey it at least shows that you can run into the problem, in which you are dependent on a slider.

@Kaydax
Copy link
Author

Kaydax commented Apr 11, 2024

So any updates on this? If there is still a problem with how I handle the inputs for zoom, it would be good to help find a better way if that's the only issue.

@ghost
Copy link

ghost commented Apr 23, 2024

So any updates on this? If there is still a problem with how I handle the inputs for zoom, it would be good to help find a better way if that's the only issue.

I have a problem, zoom simply does not work, i am on kde6. Besides that i only have a 60% keyboard and there is no way to make those combos work. Getting a full sized keyboard is not a solution either, nor having 2 keyboards on the desk at the same time.

@Kaydax
Copy link
Author

Kaydax commented Apr 28, 2024

I have a problem, zoom simply does not work, i am on kde6. Besides that i only have a 60% keyboard and there is no way to make those combos work. Getting a full sized keyboard is not a solution either, nor having 2 keyboards on the desk at the same time.

Wdym the "no way to make those combos work". The only key combos used are CTRL +/-. and 60% keyboards have +/-

@ghost
Copy link

ghost commented Apr 28, 2024

I have a problem, zoom simply does not work, i am on kde6. Besides that i only have a 60% keyboard and there is no way to make those combos work. Getting a full sized keyboard is not a solution either, nor having 2 keyboards on the desk at the same time.

Wdym the "no way to make those combos work". The only key combos used are CTRL +/-. and 60% keyboards have +/-

Those combos work only in english layout on the 60%keyboard, which i rarely use. Took me some time to find out.

Copy link
Member

@Vendicated Vendicated left a comment

Choose a reason for hiding this comment

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

you convinced me so i guess we can go forward and get this merged!

however there are some things that need to be fixed first

}}
minValue={0.5}
maxValue={2}
markers={[0.5, 0.67, 0.75, 0.8, 0.9, 1, 1.1, 1.25, 1.5, 1.75, 2]}
Copy link
Member

Choose a reason for hiding this comment

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

this constant seems duplicated across multiple files. for better maintainability, it should be moved to the shared/ folder

maxValue={2}
markers={[0.5, 0.67, 0.75, 0.8, 0.9, 1, 1.1, 1.25, 1.5, 1.75, 2]}
stickToMarkers={true}
onMarkerRender={v => (v === 1 ? "100" : `${Math.round(v * 100)}`)}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
onMarkerRender={v => (v === 1 ? "100" : `${Math.round(v * 100)}`)}
onMarkerRender={v => (v === 1 ? "100" : String(Math.round(v * 100)))}

markers={[0.5, 0.67, 0.75, 0.8, 0.9, 1, 1.1, 1.25, 1.5, 1.75, 2]}
stickToMarkers={true}
onMarkerRender={v => (v === 1 ? "100" : `${Math.round(v * 100)}`)}
></Slider>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
></Slider>
/>

@@ -18,6 +18,7 @@ export const enum IpcEvents {
FOCUS = "VCD_FOCUS",
MINIMIZE = "VCD_MINIMIZE",
MAXIMIZE = "VCD_MAXIMIZE",
SET_ZOOM = "VCD_ZOOM",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
SET_ZOOM = "VCD_ZOOM",
SET_ZOOM = "VCD_SET_ZOOM",

@@ -20,6 +20,7 @@ export interface Settings {
arRPC?: boolean;
appBadge?: boolean;
disableMinSize?: boolean;
zoomFactor?: number;
Copy link
Member

Choose a reason for hiding this comment

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

wouldn't this make a lot more sense in State than in Settings?

Copy link
Author

Choose a reason for hiding this comment

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

Then how would it persist after restarting?

Copy link
Member

Choose a reason for hiding this comment

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

state is also persisted!


function handleZoomOut() {
const zoomFactor = Settings.store.zoomFactor ?? 1;
const currentIndex = allowedZoomFactors.indexOf(zoomFactor);
Copy link
Member

Choose a reason for hiding this comment

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

if you just want to check for existence of an element, includes should be used over indexOf

Suggested change
const currentIndex = allowedZoomFactors.indexOf(zoomFactor);
const currentIndex = allowedZoomFactors.includes(zoomFactor);

defaultValue={1}
onValueChange={v => {
settings.zoomFactor = v;
VesktopNative.win.zoom(v);
Copy link
Member

Choose a reason for hiding this comment

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

you don't need this ipc call. you can register a settings change listener in the native process, look at the initSettingsListener function in mainWindow.ts

onValueChange={v => {
settings.zoomFactor = v;
VesktopNative.win.zoom(v);
setZoomFactor(v);
Copy link
Member

Choose a reason for hiding this comment

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

you don't need this state. via useSettings(), you can get a stateful settings object that will automatically rerender your component on change

the parent component already uses this hook so you should not need to call it yourself


useEffect(() => {
const handleZoomChange = event => {
console.log("zoom changed", event.detail);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
console.log("zoom changed", event.detail);

setZoomFactor(event.detail);
};

window.addEventListener("zoomChanged", handleZoomChange);
Copy link
Member

Choose a reason for hiding this comment

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

you shouldn't need this event. changing settings will already update your state & rerender the ui

Copy link
Author

Choose a reason for hiding this comment

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

I removed the listener, and the UI element does not update when the zoom gets changed, even with a listener

@Vendicated Vendicated marked this pull request as draft July 4, 2024 16:52
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.

6 participants