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

WinUI3 Window and Application API updates #71

Open
wants to merge 22 commits into
base: master
Choose a base branch
from
Open

WinUI3 Window and Application API updates #71

wants to merge 22 commits into from

Conversation

marb2000
Copy link

Adding draft of Window and Application API Specs to support Win32

Adding draft of Window and Application API Specs to support Win32
@marb2000 marb2000 requested a review from a team as a code owner March 16, 2020 15:27
@dotMorten
Copy link

dotMorten commented Mar 16, 2020

You Activate a Window to view it, but you Close it to remove it. That seems rather inconsistent. Should Close be Deactiveate or should Activate be Open ?

Also it's not clear to me whether IsVisible is true or false when the window is minimized. Should it be IsOpen is IsActivated instead ? I mean I know the doc says it returns false if minimized, but the API naming itself doesn't clearly convey it.

Also I'd use the IsVisible property to determine whether I can close it or not, but if it's minimized that wouldn't work.

Copy link

@jeffstall jeffstall left a comment

Choose a reason for hiding this comment

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

🕐

Copy link
Author

@marb2000 marb2000 left a comment

Choose a reason for hiding this comment

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

Responding to the feedback phase 1

thomasclaudiushuber and others added 3 commits August 3, 2020 15:32
* Update Window_and_Application_API_Spec.md

I don't know what the property should be called there, but definitely not Visible. :-)

* Update Window_and_Application_API_Spec.md

Change WindowSize to just Size
Updating to align more with the API spec template

### Remarks

If this was the last window to be closed for the app, the Suspending event will be raised. For a Desktop app, the application will end.
Copy link
Contributor

Choose a reason for hiding this comment

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

Which comes first, Closed or Suspending? Doesn't Suspending also get raised in Desktop apps?

// AS is an extension method in the WinRT.CastExtensions class.
// The WinRT namespace is needed to use the extension method.
var windowNative = m_window.As<IWindowNative>();
IntPtr HWND = windowNative.WindowHandle;
Copy link
Contributor

Choose a reason for hiding this comment

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

The type should be HWND rather than IntPtr

active/Win32/Window_and_Application_API_Spec.md Outdated Show resolved Hide resolved
active/Win32/Window_and_Application_API_Spec.md Outdated Show resolved Hide resolved
active/Win32/Window_and_Application_API_Spec.md Outdated Show resolved Hide resolved
active/Win32/Window_and_Application_API_Spec.md Outdated Show resolved Hide resolved

### Properties

- Handled: Gets or sets whether the event was handled. **true** if the event has been handled by the appropriate delegate; **false** if it has not.
Copy link
Contributor

Choose a reason for hiding this comment

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

What does it mean to set this to true? What is the implication?

}
```

Title bars should be have background and foreground colors based on
Copy link

Choose a reason for hiding this comment

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

"Title bars should have background"... the be is not needed


## Window.ExtendsContentIntoTitleBar

Gets or sets a value that specifies whether the default window title bar should be removed, creating more space for content.
Copy link

Choose a reason for hiding this comment

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

Rather than saying it makes more space, might it not be better and more accurate to say something like it allows the App's UI to extend to the top of the window's frame, where the titlebar would usually be.

Copy link
Member

Choose a reason for hiding this comment

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

Should also clarify that the (0,0) point for the Window.Content is now the upper left corner of the title bar area. (If true.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Recommend: yes, update wording and clarify like this.

However, even when the default system title bar is not hidden, SetTitleBar can be used to make
additional regions in your app behave like the title bar.

The following shows a Window with a no built-in title bar.
Copy link

Choose a reason for hiding this comment

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

"The following shows a Window with a no built-in title bar." remove a


The following shows a Window with a no built-in title bar.
Instead it uses the whole window as its content area. To enable features such as window dragging,
it designates a TextBlock as the title bar.
Copy link

@mdtauk mdtauk Jan 11, 2021

Choose a reason for hiding this comment

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

Do you want the example to use the TextBlock as the titlebar, or the <Grid> containing the TextBlock? If so, the Grid should also use a background color of Transparent for hit testing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Discussion: It should be the TextBlock. The other parts of the Grid could be e.g. buttons that you want to interact with.

Recommend: Add some buttons or some such to make this more clear.

@mdtauk
Copy link

mdtauk commented Jan 12, 2021

Will there be support for WinUI 3 Windows which can hide the Minimise and Maximise buttons, much like the modal dialogs seen commonly in win32 apps - as well as MessageBoxes etc? I know these will probably be ignored by UWP app windows.

win32 dialog window
Close only

Windows 10X titlebar
10X titlebar with no Maximise/Restore button

In a UWP app you must create a new
[CoreApplicationView](https://docs.microsoft.com/uwp/api/Windows.ApplicationModel.Core.CoreApplicationView),
which will automatically create a new thread and a Window on it.
See the 'Create new Window in UWP application' example.
Copy link
Member

Choose a reason for hiding this comment

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

What if I new a Window while on the Dispatcher for a CoreWindow? Could that create me a new AppWindow instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

Discussion: If you 'new' a Window it will fail, just like today (in UWP apps). AppWindow is not being supported at this time.

object for the Window when called from a UWP app. Returns null when called from a Desktop app.

```CS
public CoreDispatcher Dispatcher { get; }
Copy link
Member

Choose a reason for hiding this comment

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

So we end up with both Dispatcher and DispatcherQueue as aliases of each other? Should this property be insta-deprecated to help developers move to the preferred one instead?


### Remarks

The following events and virtual methods are not invoked when running in a Desktop app:
Copy link
Member

Choose a reason for hiding this comment

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

Maybe link over to the Project Reunion issue thread on Win32-facing activation events so folks know they're not out of luck?

Copy link
Contributor

Choose a reason for hiding this comment

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

Discussion: You can also use Win32 (hwnd) APIs for some of this.

Recommend: Reference Project Reunion issue thread.

Copy link
Author

@marb2000 marb2000 left a comment

Choose a reason for hiding this comment

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

I don't get it. This is a C# sample, and in C# WindowHandle returns IntPtr. HWND is not even a type. Why this change?

which are always created on a new thread, and which automatically create the following for the new thread:
[ApplicationView](https://docs.microsoft.com/uwp/api/Windows.UI.ViewManagement.ApplicationView),
[CoreWindow](https://docs.microsoft.com/uwp/api/Windows.UI.Core.CoreWindow),
and the Xaml [Window](https://docs.microsoft.com/uwp/api/Windows.UI.Xaml.Window).
Copy link
Member

Choose a reason for hiding this comment

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

Need to mention that Window.Current does not work for Desktop apps and always returns null.

Copy link
Contributor

Choose a reason for hiding this comment

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

Recommend: It's in the description, but add here too is good.

var syncContext = new DispatcherQueueSyncronizationContext();
SynchronizationContext.SetSynchronizationContext(syncContext);

this.ProcessEvents(); // Message pump
Copy link
Member

Choose a reason for hiding this comment

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

What class does this refer to? The only class/interface with an arity-0 ProcessEvents() that I can find is INativeWindow, but that's part of OpenTK for iOS, so probably not what we are referring to. So what is it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Discussion: this is app code.

Recommend: Show the enclosing class context.

window.Activate();
window.Closed += (_, __) => w.DispatcherQueue.InvokeShutdown();

var syncContext = new DispatcherQueueSyncronizationContext();
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
var syncContext = new DispatcherQueueSyncronizationContext();
var syncContext = new DispatcherQueueSynchronizationContext();

thread.Start();
```

> Spec note: The DispatcherQueueSyncronizationContext is a new API
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
> Spec note: The DispatcherQueueSyncronizationContext is a new API
> Spec note: The DispatcherQueueSynchronizationContext is a new API


Gets or sets a value that specifies whether the default window title bar should be removed, creating more space for content.

Setting this property to true causes the built-in title bar to be removed.
Copy link
Member

Choose a reason for hiding this comment

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

Misleading. The title bar isn't removed. (As evidenced that you can still do title-bar things, like dragging.) It just lets the app draw in the title bar area, and the system no longer draws the title bar.

Copy link
Contributor

Choose a reason for hiding this comment

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

Discussion: If you just call Extends, the input behavior of the title bar is still there.

Recommend: Clarify


**Handled**

Gets or sets whether the window activation changed event was handled. **true**
Copy link
Member

Choose a reason for hiding this comment

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

copy/pasta. (Also, same question about what "Handled" means here.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Recommend: specify Handled behavior.


**Visible**

Gets whether the window is visible or not. **true** if the window is visible; otherwise is **false**.
Copy link
Member

Choose a reason for hiding this comment

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

"is" or "was"? This is an "-ed" event, so the property presumably refers to whether the window was made visible or not, rather than referring to the window's current visibility state (which you can obtain from the Window.Visible property.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Discussion: This tells you the current state (what it changed too).

Recommend: Does this get updated when the window is minimized?

**Arguments**

Gets the arguments that are passed to the app during its launch.
In a UWP app, this is equivalent to LaunchActivatedEventArgs.Arguments.
Copy link
Member

Choose a reason for hiding this comment

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

I assume that for a Desktop app, the Arguments is the raw command line that comes after the program name (leading spaces preserved)? I.e., we don't try to parse it in any way with CommandLineToArgvW or anything like that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Recommend: yes, specify exactly what it is here (for example does it have the process name as the first argument).


// These APIs match the system Xaml versions, except the event args are now from the Xaml namespace
event Windows.Foundation.TypedEventHandler<Object,Microsoft.UI.Xaml.XamlWindowActivatedEventArgs> Activated;
event Windows.Foundation.TypedEventHandler<Object,Microsoft.UI.Xaml.XamlWindowClosedEventArgs> Closed;
Copy link
Member

Choose a reason for hiding this comment

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

Missing from spec. Did we intend to keep this? Or did we forget to delete it from the IDL?

Copy link
Contributor

Choose a reason for hiding this comment

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

Discussion: in UWP this only raises on secondary windows, not the primary window.

Recommend: add full description. Are there differences between UWP and Desktop?

with this Window. In a Desktop app this is always null.

```CS
public CoreWindow CoreWindow { get; }

Choose a reason for hiding this comment

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

There's no reason to expose CoreWindow and CoreDispatcher. If UWP apps really need these, just use the existing static methods on those types. (i.e. CoreWindow.GetForCurrentThread)

Window window = new Window();
window.Content = new TextBlock() { Text = "Hello" };
window.Activate();
window.Closed += (_, __) => w.DispatcherQueue.InvokeShutdown();

Choose a reason for hiding this comment

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

Sample feedback. Please use real variable names, not "_".

the Window's underlying HWND.

```CS
protected override void OnLaunched(Microsoft.UI.Xaml.XamlLaunchActivatedEventArgs e)

Choose a reason for hiding this comment

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

I don't understand this sample. Is it in C#, which wouldn't have the IWindowNative interface definition, or in C++, which wouldn't have this language syntax.

Copy link
Contributor

Choose a reason for hiding this comment

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

Discussion: We usually do interop samples in C++, but as it is here there's a lot of context that's not given.

Recommend: either write in C++ or provide more context for C#.


> Spec note: Application is not new to this spec, what is new are the following remarks

### Remarks
Copy link

@jeffstall jeffstall Jan 14, 2021

Choose a reason for hiding this comment

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

Should we do some refactoring here, such as creating a DesktopApplication and UwpApplication? It gets confusing if we have a lot of things that never work for Win32, and this is becoming a front and center scenario?

```cs
[webhosthidden]
[contentproperty("Content")]
unsealed runtimeclass Window

Choose a reason for hiding this comment

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

Important to specify event order. Does VisibilityChanged get fired before or after SizeChanged? Does VisibilityChanged get fired when minimized?

Copy link
Contributor

Choose a reason for hiding this comment

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

Discussion: this is specified in detail today. When exactly do events raise or not raise (what modes), and in what order?

Recommend: define and specify event ordering.

Copy link

Choose a reason for hiding this comment

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

If visibility maps to ShowWindow(SW_HIDE/SW_SHOW) and IsWindowVisible, I don't think it should fire, since a minimized app is still "visible".

which are always created on a new thread, and which automatically create the following for the new thread:
[ApplicationView](https://docs.microsoft.com/uwp/api/Windows.UI.ViewManagement.ApplicationView),
[CoreWindow](https://docs.microsoft.com/uwp/api/Windows.UI.Core.CoreWindow),
and the Xaml [Window](https://docs.microsoft.com/uwp/api/Windows.UI.Xaml.Window).
Copy link
Contributor

Choose a reason for hiding this comment

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

Recommend: It's in the description, but add here too is good.

var syncContext = new DispatcherQueueSyncronizationContext();
SynchronizationContext.SetSynchronizationContext(syncContext);

this.ProcessEvents(); // Message pump
Copy link
Contributor

Choose a reason for hiding this comment

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

Discussion: this is app code.

Recommend: Show the enclosing class context.

In a UWP app you must create a new
[CoreApplicationView](https://docs.microsoft.com/uwp/api/Windows.ApplicationModel.Core.CoreApplicationView),
which will automatically create a new thread and a Window on it.
See the 'Create new Window in UWP application' example.
Copy link
Contributor

Choose a reason for hiding this comment

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

Discussion: If you 'new' a Window it will fail, just like today (in UWP apps). AppWindow is not being supported at this time.


## Window.ExtendsContentIntoTitleBar

Gets or sets a value that specifies whether the default window title bar should be removed, creating more space for content.
Copy link
Contributor

Choose a reason for hiding this comment

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

Recommend: yes, update wording and clarify like this.


Creating a new Window in a Desktop app creates a new top level HWND.

## Window.ExtendsContentIntoTitleBar
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there some kind of AppWindowTitleBar.GetTitleBarOcclusions support?

Copy link
Contributor

Choose a reason for hiding this comment

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

Recommend: follow up


**Visible**

Gets whether the window is visible or not. **true** if the window is visible; otherwise is **false**.
Copy link
Contributor

Choose a reason for hiding this comment

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

Discussion: This tells you the current state (what it changed too).

Recommend: Does this get updated when the window is minimized?

**Arguments**

Gets the arguments that are passed to the app during its launch.
In a UWP app, this is equivalent to LaunchActivatedEventArgs.Arguments.
Copy link
Contributor

Choose a reason for hiding this comment

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

Recommend: yes, specify exactly what it is here (for example does it have the process name as the first argument).


// These APIs match the system Xaml versions, except the event args are now from the Xaml namespace
event Windows.Foundation.TypedEventHandler<Object,Microsoft.UI.Xaml.XamlWindowActivatedEventArgs> Activated;
event Windows.Foundation.TypedEventHandler<Object,Microsoft.UI.Xaml.XamlWindowClosedEventArgs> Closed;
Copy link
Contributor

Choose a reason for hiding this comment

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

Discussion: in UWP this only raises on secondary windows, not the primary window.

Recommend: add full description. Are there differences between UWP and Desktop?


Gets whether the window is visible or not. **true** if the window is visible; otherwise is **false**.

## XamlWindowSizeChangedEventArgs class
Copy link
Contributor

Choose a reason for hiding this comment

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

Recommend: specify the units

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.