On MacOS, the Quit shortcut (Command+Q/Ctrl+Q) is intercepted directly
by AppKit which emits an event on the NSApplicationDelegate bound to
NSApp.delegate. The event is caught by Qt Cocoa integration which sends
a closeEvent to the QApplication, and stops the Qt eventloop if the
closeEvent pass through.
This is problematic since we don't want to kill the eventloop right
away, and it provides a different behaviour when closing than other
ways (SIGINT/closing the window) where we redirect to libvlc_Quit() and
handle the termination of interfaces from their Deactivation callback
directly.
Instead this commit install a handler to catch Qt's closeEvent, on
every platforms, so as to redirect it to libvlc_Quit();
Co-authored-by: Pierre Lamot <pierre@videolabs.io>
The Qt window is created when code from qt.cpp's WindowOpen request the
initialisation of the vlc_window instance to the compositor, using
p_intf->p_compositor->setupVoutWindow( p_wnd, &WindowCloseCb );
WindowOpen, and the code in qt.cpp in general, handles the reference
counting of Qt's resources, hence why a destructor callback, WindowCloseCb,
is forwarded to the compositor. The compositor is unable to handle the
reference counting by itself.
When the window is destroyed, it goes through CompositorVideo callbacks
first, leading to WindowCloseCb being called from the compositor.
However, WindowCloseCb will release a reference and trigger the
destruction of the Compositor instance, and thus CompositorVideo, while
being in CompositorVideo::windowDestroy method.
In this case, it triggered use-after-free by writing m_wnd and m_destroyCb
on the CompositorVideo instance.
To prevent this, we can separate the window state from the
CompositorVideo state and modify the window destroy operation to first
notify the CompositorVideo that the window has been destroyed by its
client, and only then trigger the WindowCloseCB function from callback.
The separation of the window state is made by introducing a new
CompositorWindow structure, which is currently allocated statically in
the file since there is only ever a single window allocated, but we can
move to a dynamically allocated window state as soon as a dynamically
defined number of window is required just by changing the initialization
site and release the state in the window destroy operation.
This change simplify the constraints on CompositorVideo which doesn't
need to handle re-entrancy through its destructor, or state corruption
in the middle of the call to CompositorVideo::windowDestroy. It also
align the state handling so that a window being allocated triggers
reference increase from the window, and its destruction triggers
reference decrease from the window operation and state directly,
instead of another component tied to the reference count.
Co-authored-by: Fatih Uzunoglu <fuzun54@outlook.com>
Original discovery and fix for the UAF in MR !7241.
When binding the window to the compositor, there is no error code to
signal that allocation failed, but the code is already exception safe,
so handle it through exception handlers to avoid specific memory
handling path.
Avoids deadlocks when handling WindowEnable callback and setting of fullscreen variable on the windows' vout thread
Signed-off-by: Claudio Cambra <developer@claudiocambra.com>
It appears that d4c114ea revealed this bug in `ExpandGridView`.
I have not observed this before, because it was still dormant in
the home page without b1f3e4ef, which was added in the last minute.
Nevertheless, this should have been observable in the browse
page, but for some reason I have not observed it there either.
The new scrolling behavior has caused a lot of frustration, and
it completely broke scrolling when there are sections.
The old behavior with deceleration set to 10000 feels much better,
and fixes scrolling when there are view sections.
So that the fading edge effect does not clip the view. In fact, it still
"clips" but only outside the display margins, which I assume is wanted
anyway (see the last paragraph).
Due to the fundamental working of item layering, clipping is inevitable.
This is because in order to render an item offscreen, it is necessary to
know the boundaries of the offscreen buffer. Well, unless the buffer can
have infinite size, which I assume is not technically possible as that
would require infinite memory.
As a side note, the vertical artists list view stacked above in the left
side, and the playlist stacked above in the right side both have non-
opaque background so as to reveal window's background blur effect, but
they currently use a hack to not reveal any Qt Quick item rendered behind,
and even if this is changed in the future, we still do not want the items
in the horizontal view to appear behind. So, having the fading edge effect's
clipping is beneficial here. However, if in the future we stop using that
said hack, we need to have explicit clipping here even though the fading edge
effect clips the view, because the fading edge effect is disabled when
neither the beginning nor the end fade is enabled, which may have many
reasons including not having enough size to have fading effect, or, the
current item is the beginning item (so beginning fade is disabled) and
the user hovers the end item (so end fade is disabled).
The display "margin" grows outwards the viewport, this can also be seen in `FadingEdge`
where its `ShaderEffectSource` use the negative of the display margin for its position
(left and top margins). The hover areas must perfectly overlap with the fading areas,
we need this change to satisfy that.
I also fix mistakenly using beginning margin for the end hover area.
Normally we are not using preserve aspect crop in vertical list views, but
in this case since we want a perfect circle, we need to crop when necessary.
This is relevant for cover art that are not square.
This is to align with the grid views of other pages, where preserve aspect crop fill
mode is already used. Even though this is a horizontal list view, it uses the same
`GridItem` that is used in standard grid views, so the user naturally expects a
similar look here.
When `VLCStyle.isScreenSmall` is true, the artists pane can
not be resized. However since the horizontal resize handle is
still visible, the cursor changes and this makes it confusing
for the user whether the pane can actually be resized or not.
When `VLCStyle.isScreenSmall` is true, the playlist pane can
not be resized. However since the horizontal resize handle is
still visible, the cursor changes and this makes it confusing
for the user whether the pane can actually be resized or not.
This causes alignment issues with the header. It seems that they were added
in dec5d4b6, as part of avoiding directly referencing content item, but it
is not clear to me why at the moment and I do not remember why it was added
back then. It is likely a remnant from the idea of having a default padding
that act as the base padding and the root control's padding acting as extra
padding. There is still not a good solution for this in QML, i.e. adjusting
property when an item is reused while keeping the default value in account.
However, we have nevertheless moved away from that approach for a while, as
it is better to have full control of something under the jurisdiction of an
item where it is reused. If necessary, the default value can be repeated in
where it is reused.
That being said, I don't remember having this issue before, so it is likely
some change in the header caused this alignment issue. Perhaps the header
also contained this same amount of base padding, and it was removed there
but kept here.
This fixes a potential polish loop. I reproduce this when
I change the user scale of the interface, especially in a
rapid succession.
Having the binding delayed here makes more sense than
not having it, because it depends on something that it
also has an effect. And we already delay the visibility
bindings in layouts in other cases.
So that we don't need to use `focus: true` in every main view.
We can do this because main views are expected to request focus.
Currently views in `VideoAll` are missing this, instead of setting
them there, I thought that this approach would be less fragile.
Focus reason is a different matter and is not handled here.
Alexandre Janniaux
Claudio Cambra
Fatih Uzunoglu
Astrom Cheng
Prince Gupta
Steve Lhomme
Pierre Lamot