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

Expose screen dimensions #1519

Merged
merged 26 commits into from
Jul 16, 2022
Merged

Conversation

skamensky
Copy link
Contributor

PR for issue #1518

Status: working for linux
Upcoming changes: macos and windows implementation of get dimensions
Requested feedback before I continue working on this:

  1. api. Should this be different?
  2. Does scale need to be handled or can we just return the height and width as it is from gtk? From the docs it seems this is fine and it also seems to be how it's used for other apis in wails
  3. Did I modify all of the correct places for adding a new window call? If not, is there a comprehensive list of places to change? Are any of these auto-gererated
  4. Should this even be called window or should be add a new "namespace" of screen since technically the api is unrelated to any individual window
  5. Do we have somewhere to add tests?
  6. General feedback (this is my first time using golang)

Some other questions

When do we put c implementation in the /v2/internal/ffenestri/ffenestri_{platform}.c files and when do we put them in the /v2/internal/frontend/desktop/{platform}/window.go files?

When do we need wait groups+invokeOnMainThread?
for example I see a wait group here

func (w *Window) Size() (int, int) {
var width, height C.int
var wg sync.WaitGroup
wg.Add(1)
invokeOnMainThread(func() {
C.gtk_window_get_size(w.asGTKWindow(), &width, &height)
wg.Done()
})
wg.Wait()
return int(width), int(height)
}

But none here

func (w *Window) SetMaxSize(maxWidth int, maxHeight int) {
w.maxHeight = maxHeight
w.maxWidth = maxWidth
C.SetMinMaxSize(w.asGTKWindow(), C.int(w.minWidth), C.int(w.minHeight), C.int(w.maxWidth), C.int(w.maxHeight))
}

When do we need to add to this switch statement?

func (d *Dispatcher) processSystemCall(payload callMessage, sender frontend.Frontend) (interface{}, error) {
// Strip prefix
name := strings.TrimPrefix(payload.Name, systemCallPrefix)
switch name {
case "WindowGetPos":
x, y := sender.WindowGetPosition()
return &position{x, y}, nil
case "WindowGetSize":
w, h := sender.WindowGetSize()
return &size{w, h}, nil
case "Environment":
return runtime.Environment(d.ctx), nil
default:
return nil, fmt.Errorf("unknown systemcall message: %s", payload.Name)
}
}

@leaanthony
Copy link
Member

leaanthony commented Jul 3, 2022

Thanks for this! 🚀

To answer your questions:

api. Should this be different?

I think it's ok. The thing to keep in mind sometimes people have multiple monitors, which will affect the API.

Does scale need to be handled or can we just return the height and width as it is from gtk? From the docs it seems this is fine and it also seems to be how it's used for other apis in wails

Do you mean the DPI of the screens? This is handled behind the scenes for most scenarios. I think it's ok not to report that in this instance.

Did I modify all of the correct places for adding a new window call? If not, is there a comprehensive list of places to change? Are any of these auto-gererated

I'll add more documentation on how to do this and use this as an example for that. The only places you've left out are the darwin+windows implementations of the method but I'm assuming that's deliberate for now. These aren't auto-generated but that's not a bad idea ;-)

Should this even be called window or should be add a new "namespace" of screen since technically the api is unrelated to any individual window

I think I agree with you that it needs to be in a new "namespace". You suggested "Screen" and that's not a bad option.

Do we have somewhere to add tests?

Testing is hard for gui apps, but unit tests can be added using the <filename>_test.go convension. You can find some examples of this in the repo.

General feedback (this is my first time using golang)

You're doing well - keep going! 👍

When do we put c implementation in the /v2/internal/ffenestri/ffenestri_{platform}.c files and when do we put them in the /v2/internal/frontend/desktop/{platform}/window.go files?

Always in internal/frontend, never in ffenestri. It's legacy code left over from the alpha days but has been kept around as it has some extra code that hasn't yet made it to v2 proper.

When do we need wait groups+invokeOnMainThread?

All GUI functions should normally happen on the main thread or else they have a habit of going a bit mental. This causes issues when you need to return values as now they are happening on a different thread. The wait groups are there to coordinate this and wait until the call has completed before returning the result. See it as an async to sync mechanism.

@skamensky
Copy link
Contributor Author

Thank you for the feedback. I'm working on this. Will post back when I have something working for Linux with a new 'screen' api.

leaanthony and others added 5 commits July 4, 2022 22:35
I was getting the following errors due to some bad casts.

Gdk-CRITICAL **: 18:58:51.943: gdk_monitor_get_geometry: assertion 'GDK_IS_MONITOR (monitor)' failed
Gdk-CRITICAL **: 18:58:51.943: gdk_display_get_monitor_at_window: assertion 'GDK_IS_DISPLAY (display)' failed

This commit fixes these errors
@skamensky
Copy link
Contributor Author

@leaanthony , This PR now exposes a Screen api. As of now it only exposes a ScreenGetAll function but convenience functions can be added if needed.

Here is some sample go and js on my system (with outputs)

func (a *App) OnDomReady(ctx context.Context) {
	screens, err := runtime.ScreenGetAll(ctx)
	if err != nil {
		fmt.Printf("Error getting screens: %v\n", err)
		return
	}
	for _, screen := range screens {
		fmt.Printf("{height=%v, width=%v IsCurrent=%v, IsPrimary=%v}\n", screen.Height, screen.Width, screen.IsCurrent, screen.IsPrimary)
	}
}

Output

{height=720, width=1280 IsCurrent=true, IsPrimary=true}
{height=540, width=960 IsCurrent=false, IsPrimary=false}
runtime.ScreenGetAll().then(screens=>{
    screens.forEach(screen=>{
        console.log(screen)
    })    
})

Output

{isCurrent: true, isPrimary: true, width: 1280, height: 720}
{isCurrent: false, isPrimary: false, width: 960, height: 540}

Is this API OK now? Should I get started on Windows/MacOS, or are there some changes/a different that you'd like to see?

@@ -48,7 +48,7 @@ static void SetMinMaxSize(GtkWindow* window, int min_width, int min_height, int
GdkMonitor* getCurrentMonitor(GtkWindow *window) {
// Get the monitor that the window is currently on
GdkDisplay *display = gtk_widget_get_display(GTK_WIDGET(window));
GdkWindow *gdk_window = gtk_widget_get_window(GTK_WIDGET(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.

This should be removed from this PR and put into a different PR and a different commit. In fact, this isn't even the change I meant to introduce (however, there is a bug in this code that I have a fix for).

@leaanthony
Copy link
Member

Looking good so far 👍

@skamensky
Copy link
Contributor Author

Glad to report that the windows implementation is working!

As an aside @leaanthony how have you debugged on windows? I couldn't get the windows commant prompt or powershell to output to stdout, it simply opens the windows which is another process. I attmpeted to use procmon and windows debugger but I couldn't figure out how to view simple fmt.Printf outputs.

windows_screen_api_working

@leaanthony
Copy link
Member

I just use the -ide goland flag when generating the project and use the ide debugger

@skamensky
Copy link
Contributor Author

@leaanthony , all three platforms have been implemented. What should the next steps be?

@leaanthony
Copy link
Member

Need to find time to review it 👍

Copy link
Member

@leaanthony leaanthony left a comment

Choose a reason for hiding this comment

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

This is looking and working great! My only comment is that on MacOS, the screen height looks to be affected if the task bar is there. If I'm on a screen with a taskbar, it reports 64 pixels less than when the taskbar is hidden. I think this is because the code is using visibleFrame instead of frame. So we could either return both, or keep it consistent between the platforms (report full screen size for all platforms). Thoughts?

* // Gets the all screens. Call this anew each time you want to refresh data from the underlying windowing system.
*
* @export
* @return {Promise<{isCurrent: boolean; isPrimary: boolean; width : number height : number}>} The screens
Copy link
Member

Choose a reason for hiding this comment

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

Does this return an array of objects?

Copy link
Contributor Author

@skamensky skamensky Jul 14, 2022

Choose a reason for hiding this comment

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

Yes, this is a mistake. In fact, my IDE complains about this syntax anyway. BTW, what syntax is this documentation written in?

Copy link
Member

Choose a reason for hiding this comment

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

JSDoc, though I think you can use typescript syntax.

first.RcWork.Bottom == second.RcWork.Bottom &&
first.RcWork.Right == second.RcWork.Right &&
first.RcWork.Left == second.RcWork.Left &&
first.DwFlags == second.DwFlags
Copy link
Member

Choose a reason for hiding this comment

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

This looks like a duplicate comparison

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't know if comparing RcMonitor only instead of also comparing RcWork was enough to guarantee uniqueness of a monitor. Does RcMonitor carry enough uniqueness to drop the comparison with RcWork

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I meant you have first.DwFlags == second.DwFlags twice

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I missed that. Yes, I'll remove that.

@skamensky
Copy link
Contributor Author

#1519 (review)
Yes, I wasn't sure what the difference was, now that I understand currectly, we should use frame instead of viewframe. I agree, the meaning of Screen should stay consistent across platforms.

Overall I would like to get your feedback on the rest of the questions I left i response and clean the code up a bit (I think I have a typo in a function name somewhere, I need to find it).

Should squash the commits that I think don't contribute anything? Or would you prefer I leave the history as is?

To use frame instead of visibleframe to keep into account the space the the dock takes up
We want to include that space in the calculation in order to keep the sizes of screens consistent across platforms
It used to say it returned a single screen object. Now it says that it returns an array of screen objects
@skamensky
Copy link
Contributor Author

@leaanthony ,

I've taken care of all of the changes requested.

Anything else you'd like me to do here?

BTW, this question still stands:

Should squash the commits that I think don't contribute anything? Or would you prefer I leave the history as is?

@leaanthony
Copy link
Member

I'll check it out over the weekend. We'll squash merge so all good 👍

@leaanthony
Copy link
Member

@all-contributors please add @skamensky for code, ideas and docs 👍

@allcontributors
Copy link
Contributor

@leaanthony

I've put up a pull request to add @skamensky! 🎉

@leaanthony leaanthony merged commit 04d3541 into wailsapp:master Jul 16, 2022
@leaanthony
Copy link
Member

leaanthony commented Jul 16, 2022

Nice one! This is a fantastic contribution 🎉

@skamensky
Copy link
Contributor Author

Yay thank you for guiding me through my first open source contribution!

@leaanthony
Copy link
Member

First? Really? You did a fantastic job! 👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

2 participants