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

Gesture-event interface for touchscreens #401

Open
wants to merge 18 commits into
base: master
from

Conversation

@rundfunk47
Copy link
Contributor

commented Sep 27, 2013

This pull-request contains some of the work I did during GSoC (It's best the different features be split into separate pullreqs.).

Generally, in ScummVM widgets and dialogs have the function handleMouseDown which is called upon the active dialog when we detect a mouse-down event (EVENT_LBUTTONDOWN), for example. These commits add events for touch events that are to be interpreted by widgets. It also features widgets that are "ported" to work with gestures (i.e. scrolling lists by swiping up/down) and enabling android to use these (configure with -enable-touchmapper).

In order to get this to work and also to work with games a "touch-mapper" is used, which is an eventManager that interprets touch-events and translates these to either work with touchpad-mode in-game, direct-input in-game or to be interpreted normally in the GUI (also another plus, is that this does not need to be written from scratch for every port).

Anyway, this code seems to work fine for Android-devices, I'd love to get some feedback as well if other gestures should be added, changed, if it may pose problems with other touch-device ports etc.

@@ -131,11 +131,6 @@ void BrowserDialog::handleCommand(CommandSender *sender, uint32 cmd, uint32 data
}
break;
case kListSelectionChangedCmd:
// We do not allow selecting directories in directory

This comment has been minimized.

Copy link
@sev-

sev- Sep 29, 2013

Member

Why is this removed?

This comment has been minimized.

Copy link
@rundfunk47

rundfunk47 Oct 1, 2013

Author Contributor

Previously, clicking (or tapping) on a file would cause the list to reset its position. That behavior made it impossible to navigate through a list by dragging it with the finger, since a single tap selects the entry (and releasing at the same location would open the directory).

This comment has been minimized.

Copy link
@lordhoto

lordhoto Oct 19, 2013

Member

Wouldn't it make more sense to simply not reset the position in case this happens instead of allowing to select files instead?

MessageDialog alert(_("ScummVM couldn't open the specified directory!"));
alert.runModal();
return;
// If a file is selected, try looking for the game in the directory it resides

This comment has been minimized.

Copy link
@sev-

sev- Sep 29, 2013

Member

This is a bit unexpected. Why this change?

This comment has been minimized.

Copy link
@rundfunk47

rundfunk47 Oct 1, 2013

Author Contributor

Since tapping on a directory enters it, selecting a directory is a bit tricky. Therefore I added this to make it possible to add a game when in the directory.

This comment has been minimized.

Copy link
@wjp

wjp Oct 1, 2013

Member

Originally it wasn't possible to select a file in the dirbrowser (kListItemActivatedCmd and kListSelectionChangedCmd prevented that), but you seem to have changed that behaviour elsewhere?

@@ -31,6 +31,8 @@
#include "gui/object.h"
#include "gui/ThemeEngine.h"

#include "common/debug.h"

This comment has been minimized.

Copy link
@sev-

sev- Sep 29, 2013

Member

Please remove it from here and add to relevant .cpp file. No need to add more inter-header dependency

_currentPos--;
}
} else {
while (i <= stepSize*-1) {

This comment has been minimized.

Copy link
@sev-

sev- Sep 29, 2013

Member

why not just "-stepSize" (minus)?

@sev-

This comment has been minimized.

Copy link
Member

commented Sep 29, 2013

Pretty clean code

void handleMouseDown(int x, int y, int button, int clickCount);
void handleMouseUp(int x, int y, int button, int clickCount);
void handleMouseWheel(int x, int y, int direction);

This comment has been minimized.

Copy link
@lordhoto

lordhoto Oct 19, 2013

Member

Why is this function completely removed? Shouldn't this now break scrolling of the scollbar widget via mouse wheel?

This comment has been minimized.

Copy link
@rundfunk47

rundfunk47 Oct 23, 2013

Author Contributor

Oversight on my part, it has been reinstated


int i = _scrollOffset + pixels;

if (i > 0) {

This comment has been minimized.

Copy link
@lordhoto

lordhoto Oct 19, 2013

Member

The logic in both branches looks like it can be handled by divison and modulo. Is there any reason why this is not done?


_scrollOffset = _scrollOffset + (int)data;

if (_scrollOffset > 0) {

This comment has been minimized.

Copy link
@lordhoto

lordhoto Oct 19, 2013

Member

Again can't we use division/modulo here instead?

@@ -305,6 +305,13 @@ void ButtonWidget::handleMouseDown(int x, int y, int button, int clickCount) {
setPressedState();
}

#ifdef ENABLE_TOUCHMAPPER
void ButtonWidget::handleFingerMoved(int x, int y, int deltax, int deltay, int button) {
clearFlags(WIDGET_HILITED | WIDGET_PRESSED);

This comment has been minimized.

Copy link
@lordhoto

lordhoto Oct 19, 2013

Member

I don't understand why a finger movement will always make the button not highlighted anymore. Can you please explain this?

This comment has been minimized.

Copy link
@rundfunk47

rundfunk47 Oct 23, 2013

Author Contributor

Holding a finger over a button, then dragging outside of it should deselect it (although, this is a bit of an unclean way of doing it, a better way would probably be to implement a function e.g. handleFingerLeft that would take care of this.. I'll have a look at it).

int deltax;
int deltay;

int holdTime;

This comment has been minimized.

Copy link
@lordhoto

lordhoto Oct 19, 2013

Member

I figure this is the time the finger has been pressed in milliseconds, to be consistent with the way we return the overall time it would make more sense to me to use uint32 here.


int holdTime;

byte flags;

This comment has been minimized.

Copy link
@lordhoto

lordhoto Oct 19, 2013

Member

What flags are this? This is missing documentation in my eyes.

This comment has been minimized.

Copy link
@rundfunk47

rundfunk47 Oct 23, 2013

Author Contributor

Was planning on using these, but they have been removed now

/**
* Position of the finger, in virtual screen coordinates.
* Virtual screen coordinates means: the coordinate system of the
* screen area as defined by the most recent call to initSize().

This comment has been minimized.

Copy link
@lordhoto

lordhoto Oct 19, 2013

Member

Hm does this mean it always returns game screen coordinates even when the overlay is visible?

@@ -67,6 +68,10 @@
#else
_dispatcher.registerMapper(new Common::DefaultEventMapper());
#endif
#ifdef ENABLE_TOUCHMAPPER
_touchmapper = new Common::Touchmapper(this);
_dispatcher.registerMapper(_touchmapper);

This comment has been minimized.

Copy link
@lordhoto

lordhoto Oct 19, 2013

Member

Since this call effectively replaces the old event mapper shouldn't this break the keymapper in case it's compiled in (at least I cannot find any other code changes which seem to avoid it)? I guess we would need to make sure the touchmapper and keymapper are mutually exclusive (this probably also means we cannot enable the touchmapper and event recorder at the same time, however, since the event recorder is desktop/SDL only this should not be too much of a problem hopefully. It probably still makes sense to check for this case though).

// Triggered when finger leaves screen
EVENT_TAPUP = 24,
EVENT_DOUBLETAP = 25,
EVENT_SIMULATE_LBUTTONDOWN = 26,

This comment has been minimized.

Copy link
@lordhoto

lordhoto Oct 19, 2013

Member

I take all these "simulate" events are some kind of internal events from the backend to the touchmapper? I think we should make sure it's documented to avoid frontend people being worried about how to handle them.

This comment has been minimized.

Copy link
@rundfunk47

rundfunk47 Oct 23, 2013

Author Contributor

Yes, that's the idea, since different platforms might have different ways of simulating left/right mouse-clicks (e.g. one finger doubletap, two finger tap, modifiers etc..).

@sev-

This comment has been minimized.

Copy link
Member

commented Apr 28, 2014

Could you please bring this up to date? I'd like to finally merge this.

@rundfunk47

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2014

Absolutely!

@RichieSams RichieSams force-pushed the scummvm:master branch from 807d18e to 2ec3adf Sep 12, 2014
@digitall digitall force-pushed the scummvm:master branch from 2208eda to 5b5a2bc Oct 26, 2014
@sev-

This comment has been minimized.

Copy link
Member

commented Jan 4, 2015

Any update on this?

@sev-

This comment has been minimized.

Copy link
Member

commented Nov 7, 2015

What about now? @rundfunk47, I really hope you will find some time for bringing it up to date so I could merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.