-
-
Notifications
You must be signed in to change notification settings - Fork 193
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
base: main
Are you sure you want to change the base?
Conversation
@@ -470,6 +503,32 @@ export async function createWindows() { | |||
mainWin!.maximize(); | |||
} | |||
}); | |||
|
|||
mainWin.on("focus", () => { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
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 |
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 |
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 |
This comment was marked as spam.
This comment was marked as spam.
The zoom shortcut does not seem to work on linux (Vesktop and/or official client) |
@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. |
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. |
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. |
There was a problem hiding this 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]} |
There was a problem hiding this comment.
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)}`)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
></Slider> | |
/> |
@@ -18,6 +18,7 @@ export const enum IpcEvents { | |||
FOCUS = "VCD_FOCUS", | |||
MINIMIZE = "VCD_MINIMIZE", | |||
MAXIMIZE = "VCD_MAXIMIZE", | |||
SET_ZOOM = "VCD_ZOOM", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SET_ZOOM = "VCD_ZOOM", | |
SET_ZOOM = "VCD_SET_ZOOM", |
@@ -20,6 +20,7 @@ export interface Settings { | |||
arRPC?: boolean; | |||
appBadge?: boolean; | |||
disableMinSize?: boolean; | |||
zoomFactor?: number; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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
const currentIndex = allowedZoomFactors.indexOf(zoomFactor); | |
const currentIndex = allowedZoomFactors.includes(zoomFactor); |
defaultValue={1} | ||
onValueChange={v => { | ||
settings.zoomFactor = v; | ||
VesktopNative.win.zoom(v); |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
console.log("zoom changed", event.detail); |
setZoomFactor(event.detail); | ||
}; | ||
|
||
window.addEventListener("zoomChanged", handleZoomChange); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
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.