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

[UX] Gesture manager: corner gestures #4878

Merged
merged 20 commits into from Apr 7, 2019
Merged
Changes from 17 commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -37,6 +37,8 @@ read_globals = {
"DTAP_ZONE_BACKWARD",
"DTAP_ZONE_BOOKMARK",
"DTAP_ZONE_FLIPPING",
"DTAP_ZONE_BOTTOM_LEFT",
"DTAP_ZONE_BOTTOM_RIGHT",
"DDOUBLE_TAP_ZONE_NEXT_CHAPTER",
"DDOUBLE_TAP_ZONE_PREV_CHAPTER",
"DCHANGE_WEST_SWIPE_TO_EAST",
@@ -73,8 +73,12 @@ DTAP_ZONE_CONFIG = {x = 1/8, y = 7/8, w = 3/4, h = 1/8}
DTAP_ZONE_MINIBAR = {x = 0, y = 31/32, w = 1, h = 1/32}
DTAP_ZONE_FORWARD = {x = 1/4, y = 0, w = 3/4, h = 1}
DTAP_ZONE_BACKWARD = {x = 0, y = 0, w = 1/4, h = 1}
-- top right corner
DTAP_ZONE_BOOKMARK = {x = 7/8, y = 0, w = 1/8, h = 1/8}
-- top left corner
DTAP_ZONE_FLIPPING = {x = 0, y = 0, w = 1/8, h = 1/8}

This comment has been minimized.

Copy link
@poire-z

poire-z Apr 6, 2019

Contributor

Can't you migrate these two to DTAP_ZONE_BOTTOM_LEFT like the other ones? And use where they are used

if DTAP_ZONE_BOOKMARK  then -- legacy global variable, may still be defined in default.persistent.lua
  -- use DTAP_ZONE_BOOKMARK
else
  -- use DTAP_ZONE_TOP_RIGHT
end

or luacheck would complain ?

This comment has been minimized.

Copy link
@Frenzie

Frenzie Apr 6, 2019

Author Member

No, should be fine.

DTAP_ZONE_BOTTOM_LEFT = {x = 0, y = 7/8, w = 1/8, h = 1/8}
DTAP_ZONE_BOTTOM_RIGHT = {x = 7/8, y = 7/8, w = 1/8, h = 1/8}
DDOUBLE_TAP_ZONE_NEXT_CHAPTER = {x = 6/8, y = 0, w = 2/8, h = 2/8}
DDOUBLE_TAP_ZONE_PREV_CHAPTER = {x = 0, y = 0, w = 2/8, h = 2/8}

@@ -88,7 +88,7 @@ function FileManager:init()
padding_left = Size.padding.large,
padding_right = Size.padding.large,
padding_bottom = 0,
callback = function() self:tapPlus() end,
callback = nil, -- top right corner callback handled by gesture manager

This comment has been minimized.

Copy link
@poire-z

poire-z Apr 6, 2019

Contributor

Might be a first (drawing a button, no callback, but having it somethow triggered by some remote overlay, hoping the regions of each will keep overlaping as we make changes :)
But I don't see how else easily...

This comment has been minimized.

Copy link
@Frenzie

Frenzie Apr 6, 2019

Author Member

I could take it back out again. Maybe I'll think of something after a night's sleep. ;-)

This comment has been minimized.

Copy link
@Frenzie

Frenzie Apr 7, 2019

Author Member

Btw, not really a first. This is basically exactly what the flipping & bookmark corners do.

This comment has been minimized.

Copy link
@poire-z

poire-z Apr 7, 2019

Contributor

They do not feel like that to me. ReaderDogear and ReaderFlipping widgets both are painted (and so are containers of their own inner widgets), set the tap zone, and handle the action.

This comment has been minimized.

Copy link
@Frenzie

Frenzie Apr 7, 2019

Author Member

They might technically set the tap zone, but the tap zone they set is completely unrelated to the actual widget.

if Device:isTouchDevice() then
self.ges_events = {
Tap = {
GestureRange:new{
ges = "tap",
range = Geom:new{
x = new_screen_width*DTAP_ZONE_FLIPPING.x,
y = new_screen_height*DTAP_ZONE_FLIPPING.y,
w = new_screen_width*DTAP_ZONE_FLIPPING.w,
h = new_screen_height*DTAP_ZONE_FLIPPING.h
}
}
}
}
end

if Device:isTouchDevice() then
self.ges_events = {
Tap = {
GestureRange:new{
ges = "tap",
range = Geom:new{
x = new_screen_width*DTAP_ZONE_BOOKMARK.x,
y = new_screen_height*DTAP_ZONE_BOOKMARK.y,
w = new_screen_width*DTAP_ZONE_BOOKMARK.w,
h = new_screen_height*DTAP_ZONE_BOOKMARK.h
}
}
}
}
end

This comment has been minimized.

Copy link
@poire-z

poire-z Apr 7, 2019

Contributor

I was not referring about tap zone overlaping with the icon. Just the self containment of the 3 parts involved (widget, tap zone, action) in a single InputContainer.
What bothers me is the "remote overlay", the tap zone being set far away in readergesture.
But now, it's no more a first, but a second and a third :)
But again, I don't see how else.

This comment has been minimized.

Copy link
@Frenzie

Frenzie Apr 7, 2019

Author Member

True, it's nice that they're self-contained units.

I think the "proper" way would be to allow the user to set their own icon+action+possibly tap zone in place (including one that's transparent) so that they are more together.

In Opera's toolbar.ini that'd look like this:

[Document Toolbar.content]
NamedButton0, tbb_Back, -847360260="Back + Show hidden popup menu, "Back Menu""
NamedButton1, tbb_Forward, 830055518="Forward + Show hidden popup menu, "Internal Forward History""
Button2, -108388079="Fast Forward, , , "Next" + Show hidden popup menu, "Internal Fast Forward History""
Button3, -119414254="Wand | Fast Forward, , , "Next" + Show hidden popup menu, "Internal Fast Forward History""
Address4
Button5, "''InO"="Execute program, "opera-developer", "%u", "InO", "OperaBlink""
Button6, "InFx"="Execute program, "firefox", "%u", "InFx", "Firefox""
Zoom7
ExtensionSet8

In Otter it's done with JSON:

[
	{
		"actions": [
			"RewindAction",
			"GoBackAction",
			"GoForwardAction",
			"FastForwardAction",
			"ReloadOrStopAction",
			"AddressWidget",
			"SearchWidget"
		],
		"buttonStyle": "iconOnly",
		"fullScreenVisibility": "hidden",
		"identifier": "AddressBar",
		"normalVisibility": "visible",
		"row": 0,
		"title": "Address Bar"
	},
	{
		"buttonStyle": "iconOnly",
		"currentPanel": "bookmarks",
		"fullScreenVisibility": "hidden",
		"identifier": "SideBar",
		"location": "left",
		"normalVisibility": "visible",
		"panelSize": 530,
		"panels": [
			"bookmarks",
			"cookies",
			"history",
			"notes",
			"passwords",
			"transfers",
			"windows",
			"config"
		],
		"row": 0,
		"title": "Sidebar"
	}
]

That way you've got clear separation of concern in the code but everything is also clearly defined together and configurable. We've currently got a bit of an awkward in-between.

Also I don't have a clear vision yet for the outcome, but I do know that with all the actions that've been added to the gesture manager it's much easier to move to a different/further abstraction than it was a few months ago.

This comment has been minimized.

Copy link
@poire-z

poire-z Apr 7, 2019

Contributor

Well, I hope we don't need all that :)
Also, we're in a bit different situation as the code is in Lua, so we don't really need to provide as much hooks for customisation as the apps in compiled C that are not hackable - the interested users (very little) can always hack/patch/monkey-patch our code base to have things a bit more to their liking (I do that). Complexifying and abstracting with different levels of indirection to satisfy a small user base is not really to my liking :)

But yes, with gesturemanager, it has become different - you could possibly move everything reader/filemanager gestures in there to have them in a single place. But that would be one global level of indirection that would make reading/hacking the code a bit more tedious.

This comment has been minimized.

Copy link
@Frenzie

Frenzie Apr 7, 2019

Author Member

Architecturally speaking it may be better for the gesture manager to be more like the plugin manager. Actually it's already quite a bit like that with the event system. But anyway, it's working and I have no special desire to waste time redesigning it or whatnot. ;-)

}

self.path_text = TextWidget:new{
@@ -404,6 +404,11 @@ function FileChooser:onBack()
end
end

function FileManager:onShowPlusMenu()
self:tapPlus()
return true
end

function FileManager:tapPlus()
local buttons = {
{
@@ -1,11 +1,9 @@
local Device = require("device")
local Event = require("ui/event")
local Geom = require("ui/geometry")
local GestureRange = require("ui/gesturerange")
local ImageWidget = require("ui/widget/imagewidget")
local InputContainer = require("ui/widget/container/inputcontainer")
local RightContainer = require("ui/widget/container/rightcontainer")
local Screen = require("device").screen
local Screen = Device.screen

local ReaderDogear = InputContainer:new{}

@@ -70,30 +68,9 @@ end
function ReaderDogear:resetLayout()
local new_screen_width = Screen:getWidth()
if new_screen_width == self._last_screen_width then return end
local new_screen_height = Screen:getHeight()
self._last_screen_width = new_screen_width

self[1].dimen.w = new_screen_width
if Device:isTouchDevice() then
self.ges_events = {
Tap = {
GestureRange:new{
ges = "tap",
range = Geom:new{
x = new_screen_width*DTAP_ZONE_BOOKMARK.x,
y = new_screen_height*DTAP_ZONE_BOOKMARK.y,
w = new_screen_width*DTAP_ZONE_BOOKMARK.w,
h = new_screen_height*DTAP_ZONE_BOOKMARK.h
}
}
}
}
end
end

function ReaderDogear:onTap()
self.ui:handleEvent(Event:new("ToggleBookmark"))
return true
end

function ReaderDogear:onSetDogearVisibility(visible)
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.