Bug 1032

Created attachment 674 [details]
Fix keyboard shortcuts

I recommend the following shortcut changes:

Pause = spacebar (consistent with other media apps)
Play = P (current shortcut is R)

This requires changing the menu as well as source code.

I am attaching a patch.

The pause shortcut works in full view mode, which addresses the original problem report because "P" no longer is the primary shortcut for pausing and the patch explicitly defines only the Space key as the pause shortcut.

The S(top) and P(lay) shortcuts do not work in full view mode. Both shortcuts work in normal view mode. I am updating the Summary description accordingly.

Proposed patch works properly.
We could even add 'M' to switch to/from minimal mode?

That probably is a GOOD idea. I just tested minimal mode but could not figure out how to restore back to normal mode. While in minimal mode, pressing F initiates full view mode, but pressing F again returns to minimal mode rather than normal mode. Yuck.

Do you want to add that? I'll test.

Created attachment 688 [details]
Fix keyboard shortcuts, add minimal mode to popup menu

At first I was a bit tangled in the slots / signals / object hierarchy / need to call a method from the parent object in KMPlayer... But be succeeded. Apart from adding shortcut for minimal mode, I added a minimum mode into popup menu. I also move item for fullscreen up - before scale to popup menu was in the same order as in the menubar.

Please test it. I tested in 3.5.13.1.

I noticed one thing - when you switch to minimal mode and back is not available the bottom control bar. However, as far as I know, this problem has been before.

The new M shortcut works and appears in the menu. I noticed a few quirks about this minimal view:

* When toggling from normal view to minimal view, kmplayer loses focus. I have to select the window with the mouse to regain focus before I can press M to return to normal view. Using the mouse kind of defeats using shortcuts. :-)

* In minimal mode, kmplayer does not appear in the Alt-Tab window list. Therefore only the mouse can be used to regain focus. That would be two bugs: 1) losing focus, and 2) not appearing in the window list.

* With no video loaded or viewing small width videos (240p), and returning to normal view from minimal view, the window is not restored to the user's initial size but instead to a size that causes the menu bar to wrap to two rows. The problem is caused by View->Keep Width/Height Ratio being enabled. I disabled that option but kmplayer re-enables when returning to normal view from minimal view. This behavior will frustrate people.

* As you noticed, when returning to normal view from minimal view, the player controls do not reappear. I got the controls to reappear by, after returning to normal view from minimal view, toggling to full view and back to normal view.

First patch from attachment 688 [details] pushed to GIT in hash 791704c5.

When switching to minimal mode the window type is changed from NET::Normal to NET::Menu / NET::Utility (for thinner title). During this is hidden / set zoom 100% / showed (see kmplayerapp.cpp:1238-1252). As a consequence window is removed from the Alt+Tab (due to NET::Menu / NET::Utility) and loses focus (due to hide / show).

If he did not change the window type, probably would not needed to hide / show (not loses focus) and would not disappear from the Alt + Tab.

We want it?

My perspective:

The current behavior is inconsistent with other Trinity apps. Changing to Minimal View should not lose focus or be removed from the Alt-Tab window list.

To me, both events defeat the purpose of using keyboard shortcuts. When in Minimal View mode, losing focus means needing to use the mouse to control the app because the Alt-Tab window list does not contain the app. When in Minimal View there is no keyboard method available to control the app because of the loss of focus and loss of Alt-Tab listing.

This is dangerous too because many users likely will not recognize the app has lost focus. Any keyboard shortcut the user presses will affect the app that has focus and not to kmplayer. That type of bug is very much like the bug we had with kate not receiving focus.

I also notice that even when the user uses the mouse to invoke Minimal View, the app still loses focus.

To me all of this is inconsistent and likely will confuse just about any user.

Seems we need to retain focus whether or not the Minimal View keyboard shortcut is added. By retaining focus we also resolve the Alt-Tab list problem.

To me this was just bad design from the beginning. :-)

Created attachment 706 [details]
Disable changes of the window type

Proposed patch just disables changes of the window type. And because it is not necessary to hide / show => window not lose the focus.

When I build with the attachment 706 [details] patch, what do I test as far as usability?

The patch to disable the window type seems to have resolved the quirks with minimal view mode. Whether using the keyboard shortcut 'M' or the mouse, when invoking minimal view mode the app does not lose focus and the app remains in the Alt-Tab app list.

By the way, by accident, when I hovered the mouse pointer to the area just barely above the bottom border, the player controls appeared. As long as the pointer remains in the player control the controls remain. Move the pointer outside the controls and they disappear.

One usability quirk is when in minimal view mode, unless the user remembers the keyboard shortcut 'M', there does not seem to be any way to get out of minimal view mode. Double-clicking with the mouse button invokes full screen mode and double-clicking again returns to minimal mode. This is not a bug per se, just a quirk.

I would say push both patches and tag this bug report as resolved.

Showing bar with controls at bottom border in minimal mode is right - that's what the authors intended.

Note that in the menu that appears on the first button in this bar (or as context menu in window) is added item for switching to / from minimal mode. (Is added in patch from attachment 688 [details].)

There is still a question whether at the switch to / from minimal mode is desirable to change the display size? At present, the mode switch forces a scale of 100%.

Remains unresolved, in fullscreen player ignores the settings of shortcuts => are hard-coded in src/xineplayer.cpp, lines from 836. And also that in fullscreen is xine controlled directly, without use PartBase => without the rest of the KMPlayer functions.

I am not objecting to how the player controls appear and disappear while in minimal mode. I only intended to share my observation that getting the controls to appear is possible. :-)

When invoking minimal mode, rescaling to the size of the video somewhat makes sense. I don't mind that. However, the app should always remember the size of the window before invoking minimal mode so when users toggle out of minimal mode to normal mode the window is restored to the user's preferred size.

An irritating part is sometimes kmplayer starts with menu bar wrapping to two rows. I haven't figured out how to duplicate all the time.

Unless you think the keyboard shortcut problem in full screen mode is fixable, perhaps we should push the patches in this bug report and move on to other bug reports? Even if you want to fix the full screen shortcut problem, perhaps we should push the patches here anyway?

Kaffeine when switched normal <=> minimal mode leave window size unchanged. This also avoids having to remember the window size before / after the change. That's why I suggested the same solution for KMPlayer. :)

I propose to incorporate the existing patch (if we agree, with the abolition of resizing). And problem with keyboard shortcuts in fullscreen to postpone for later.

Sounds good to me. :-)

(Odpověď na komentář #15)
> Sounds good to me. :-)

With the abolition of resizing?

Yes, if you mean with the same behavior as kaffeine, then that sounds good to me.

Resizing often seems problematic to me. As you noted, resizing to the size of the video requires remembering the original window size. Leaving the window size as is and only removing the controls, menu bars, etc. seems a safer and saner approach.

And yes, postpone the full screen shortcut problems to later or create a new bug report to address only those bugs.

Created attachment 712 [details]
Removed window changes when switching normal / minimal mode

Please test it. When switching from the minimal mode to normal even so, (depending on the height of the window) may enlarge it :(

Might be useful to add a keyboard shortcut for setting 100% scale?
(In Kaffeine is on default Alt+1)

Latest patch works great.

Regarding a shortcut to resize to the video, if we do that then like kaffeine, should we add full support like kaffeine (Alt+0, Alt+1, Alt+2, Alt+3)? Further, when the user uses Alt+1 to force minimal mode to resize the window to the size of the video, how does the user restore the original window size?

Created attachment 781 [details]
Added next preset scales and shortcuts for zoom

I added a third patch that adds menu (and popup menu) items for next preset scales and keyboard shortcuts for zoom in compliance with Kaffeine.

The new Alt+2 and Alt+3 shortcuts work here.

So I can push these two patches into GIT? And problem with keyboard shortcuts in fullscreen (hard-coded in src/xineplayer.cpp) to postpone for
later?

Go ahead and push.

Second patch from attachment 712 [details] pushed to GIT in hash d306f1c8.
Third patch from attachment 781 [details] pushed to GIT in hash b929f2e6.

Slavek, should we tag this bug report as resolved?

There is still an unresolved problem with keyboard shortcuts in full screen (hard-coded in src / xineplayer.cpp). At the present time, but I do not have enough time to find a solution.