[sailfish-browser] Use a press and hold scheme in tab list to switch … #959
Conversation
There was a problem hiding this comment.
Almost there I'd say. Like discussed during the community meeting, people have gotten used to the close button that can be found from the toolbar menu. That's my favorite as well.
Maybe the biggest concern that I have is whether this actually improves situation or not but I'll to elaborate a way that should render good end results. I think we should leave close icon still to the TabItem header (where it was, at least for time being) as that would be a bit quicker way to close. Then this PR & commit would be more about introducing a housekeeping mode for tabs where closing is first action and re-organizing tab order could be implemented later.
| enabled: !closingAllTabs | ||
|
|
||
| onClicked: { | ||
| if (tabGridView._tabScale == 1.0) { |
There was a problem hiding this comment.
| if (tabGridView._tabScale == 1.0) { | |
| if (tabGridView.housekeeping) { | |
| tabGridView.housekeeping = false | |
| } else { | |
| activateTab(index) | |
| } |
Usually reading positive conditions are more readable than going through negative (if not housekeeping). Hence, I'd change order as well.
There was a problem hiding this comment.
Yep, I've changed all use of _tabScale to use housekeeping instead and reverse order when necessary to avoid negative in tests.
| } | ||
| onPressAndHold: tabGridView._tabScale = 0.9 | ||
| } | ||
| Rectangle { |
There was a problem hiding this comment.
This Rectangle & Image & MouseArea should really be an Rectangle & IconButton. IconButton will handle highlighting of the icon. The background Rectangle can be pretty much like already declared (small remark below).
There was a problem hiding this comment.
Good advice, thanks. I'm using an IconButton as done before in TabItem.qml.
| radius: width / 2. | ||
| anchors.horizontalCenter: parent.horizontalCenter | ||
| anchors.verticalCenter: parent.bottom | ||
| opacity: tabGridView._tabScale < 1.0 ? 1.0 : 0.0 |
There was a problem hiding this comment.
| opacity: tabGridView._tabScale < 1.0 ? 1.0 : 0.0 | |
| opacity: tabGridView.housekeeping ? 0.0 : 1.0 |
| MouseArea { | ||
| id: mouseArea | ||
| anchors.fill: parent | ||
| enabled: !closingAllTabs && tabGridView._tabScale < 1.0 |
There was a problem hiding this comment.
This enabled would be then enabling of the IconButton.
| property color highlightColor: Theme.colorScheme == Theme.LightOnDark | ||
| ? Theme.highlightColor | ||
| : Theme.highlightFromColor(Theme.highlightColor, Theme.LightOnDark) | ||
| // In direction so that we can break this binding when closing a tab | ||
| implicitWidth: width | ||
| implicitHeight: height | ||
|
|
||
| enabled: !destroying | ||
| function closing() { |
There was a problem hiding this comment.
| function closing() { | |
| function closing() { |
Closing is a state/status whereas function name should be describing what this function does (verb). I would not call this "close" either as this is not really closing anything. That said, I'd propose "markClosed" as this is changing TabItem to itself final state.
There was a problem hiding this comment.
Ok, I've changed the name for your suggestion.
dcaliste
force-pushed
the
close
branch
2 times, most recently
from
48f097f to
4111f54
Compare
|
Thank you @rainemak for your review and suggestions. I've updated the PR accordingly. I'm a bit wondering though : do you prefer to have this code to TabItem.qml ? If so, I can do, no problem. |
|
I guess rebase went wrong, maybe you rebased from master instead of next. |
|
Oops, stupid me. Corrected, now based on next, and not anymore 55 commit to push ;-) |
| @@ -110,31 +111,6 @@ BackgroundItem { | |||
| truncationMode: TruncationMode.Fade | |||
| color: down || activeTab ? root.highlightColor : Theme.primaryColor | |||
| } | |||
|
|
|||
| IconButton { | |||
There was a problem hiding this comment.
I'd keep this close button as well until housekeeping mode has also re-organizing tabs functionality.
There was a problem hiding this comment.
Ok, thanks. I understand your point now. I'll update the PR after my vacations are over, second half of August.
|
It's now rebased with all remarks addressed (if I didn't missed any) and the close icon back on TabItem. |
…to close mode.
Coming from a discussion in the forum (https://forum.sailfishos.org/t/browser-redesign-in-sailfish-4-2-feedback-thread/7867/78 and later posts), it was proposed to use a press and hold behaviour to activate a close tab mode in the tab list view. Like in the home screen (Lipstick switcher).
I've investigated the possibility and created a PR to test the idea. It's using the same metrics (animation duration, scaling factor...) than in the Lipstick switcher.
Here is a screen shot :
Entering this mode would allow later to implement a tab reordering capability, like in the Lipstick switcher.
@rainemak or @jpetrell , what do you think ?
PS : @rainemak , I've removed the timer in
TabItem.qmlthat you added in 2014 commit bab0801 when the tabs were using shader effects and textures. I think it's safe to remove it now that we're using images. Do you agree ?