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

Implement notification dismissal #323

Closed

Conversation

DavidVentura
Copy link
Contributor

@DavidVentura DavidVentura commented May 2, 2021

This fixes #176 and also fixes #272

Issues with swiping worked around by using Double-Tap instead of swipe

doc_2021-05-04_14-05-35.mp4

@ildar
Copy link

ildar commented May 2, 2021 via email

@DavidVentura
Copy link
Contributor Author

I guess this wouldn't work with different types of touch sensors often
found in P8 watches.

Why not? (I don't really know).

I have a P8 on the way (sealed), I am not very aware of the differences between models though

@DavidVentura DavidVentura force-pushed the dismiss-notifications branch from 72171e6 to cb81a9b Compare May 2, 2021 16:21
@Avamander
Copy link
Collaborator

The feature seems nice, but the scroll direction a bit wrong.

@pfeerick
Copy link
Contributor

pfeerick commented May 3, 2021

How is it wrong? Swiping down is going to move the next message (without clearing it), swiping up will hide the notifications panel. Swiping left or right is how some phone email clients are exposing the delete or other options related to a message.

What would you suggest as an alternative?

@DavidVentura DavidVentura changed the title RFC: Implement Swipe-to-dismiss on notifications RFC: Implement notification dismissal May 3, 2021
@DavidVentura
Copy link
Contributor Author

The feature seems nice, but the scroll direction a bit wrong.

Now it is dismiss on double tap; should be less jarring (video updated)

@DavidVentura DavidVentura changed the title RFC: Implement notification dismissal Implement notification dismissal May 13, 2021
@Batcastle
Copy link

Is it possible to add a setting that allows switching between swipe to dismiss and tap to dismiss? I understand for some sensors one may work better than another, but I think making which method it uses into a setting would be good for those with certain mobility issues or those who want something more familiar.

I understand this may be a lot to ask but I honestly think making it a setting would be the ideal solution.

@danielrparks
Copy link

danielrparks commented Jun 22, 2021

Is it possible to add a setting that allows switching between swipe to dismiss and tap to dismiss? I understand for some sensors one may work better than another, but I think making which method it uses into a setting would be good for those with certain mobility issues or those who want something more familiar.

Currently there is not support for persistently saving settings, but this would become possible if either #162 or #438 is merged.

Edit: there is limited support for saving settings to flash. See https://github.com/JF002/InfiniTime/blob/develop/src/components/settings/Settings.cpp for implementation.

@Riksu9000
Copy link
Contributor

The feature seems nice, but the scroll direction a bit wrong.

Now it is dismiss on double tap; should be less jarring (video updated)

I don't think the issue ever was the gesture. The issue was that swiping left caused the screen to move down. I think the swipe gesture should be brought back but the animation should be changed.

Just changing the FullRefreshDirections to Left doesn't work. The screen would have to be turned black in between, then the animation would be visible, and look as it should.

@ObiKeahloa
Copy link
Contributor

I know this might make it harder but is it possible to:
1: Swipe all the way down and get an option to clear ALL the notifications
2: Swiping right/left to be able to delete the message with a button that says "Delete" and is red in colour

@JF002
Copy link
Collaborator

JF002 commented Jul 24, 2021

Is this PR still work in progress?
I've just tested it and it seems to work fine. However, as @pfeerick and @Riksu9000 mentioned, swiping left/right seems more intuitive to me than the double tap. Why did you change it?
Regarding P8 support : If some P8 are equipped with a touch sensor that does not support gestures (up/down/left/right swipes), InfiniTime will be unusable anyway, as gestures as extensively used in InfiniTime.

@danielrparks
Copy link

If I understand correctly, the issue is that during a swipe to the side, the screen would scroll down, which felt unnatural.

I think using the settings screen transition during notification dismissal might help with that.

@Avamander
Copy link
Collaborator

Maybe long tap to dismiss? Prompt "All" or "Single" even?

@DavidVentura
Copy link
Contributor Author

Is this PR still work in progress?

Not really, although if something needs to be adjusted I can do that

swiping left/right seems more intuitive to me than the double tap. Why did you change it?

Swiping to the side should animate the notification going to the side as well, but that was a lot slower and it felt really bad IMO

@Riksu9000
Copy link
Contributor

Swiping to the side should animate the notification going to the side as well, but that was a lot slower and it felt really bad IMO

How did you implement it? Swiping left and right between watchface and quicksettings is fast so why wouldn't this be? It's technically not moving, but it gives the same feeling.

@JF002
Copy link
Collaborator

JF002 commented Jul 24, 2021

Swiping to the side should animate the notification going to the side as well, but that was a lot slower and it felt really bad IMO

I agree, we cannot move the notification laterally is a way that would be visually nice, unfortunately. I was thinking about using the same animation than for the quick setting menu, for example.

However, what's everyone's opinion? What do you think of the current UI (double tap to dismiss a notification) ? Maybe it's good enough in this current state?

@danielrparks
Copy link

danielrparks commented Jul 24, 2021

Since I would dismiss notifications a lot, I would prefer the faster gesture (the swipe) with the settings animation.

@Riksu9000
Copy link
Contributor

I agree, we cannot move the notification laterally is a way that would be visually nice, unfortunately. I was thinking about using the same animation than for the quick setting menu, for example.

I think the animation used in quicksettings would work well here too if implemented well. It could even be thought of as the notification getting shredded, which would be appropriate.

@JF002 JF002 added this to the Version 1.4 milestone Jul 28, 2021
@JF002
Copy link
Collaborator

JF002 commented Aug 4, 2021

@DavidVentura what's your opinion about this transition effect with the swipe gesture?

@Riksu9000 Riksu9000 mentioned this pull request Aug 19, 2021
@geekbozu
Copy link
Member

Any updates on this?

@DavidVentura
Copy link
Contributor Author

DavidVentura commented Aug 21, 2021 via email

@Riksu9000
Copy link
Contributor

As I've mentioned earlier, I don't think just changing the animation direction is enough. The animation will be barely visible because the screen barely changes, so I think a black frame needs to be added in between showing the two notifications. Then it would be slide left animation to black, then slide up/down the new notification.

#323 (comment)

It's been some time since I tested this, so I don't exactly remember what the difference between Left and LeftAnim is, but I don't think it solved this issue.

If you don't want to do this, then it's fine, but I would hope this gets improved in the future.

running = false;
}
}
return false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should return true, because the touch event was handled in app.

@JF002 JF002 removed this from the Version 1.4 milestone Sep 2, 2021
@geekbozu geekbozu added enhancement Enhancement to an existing app/feature needs more work This PR needs more work labels Oct 3, 2021
@Joshndroid
Copy link

Is this still being looked at?... first time looking at this thing and was swiping like an idiot to delete numerous notifications which lead me to here lol

@jzbor
Copy link

jzbor commented Dec 28, 2021

Would it be possible to first implement it with a double tap to get the feature at all and then figure out a proper way to animate it with gestures. It seems to me like a not so important detail is holding back this pretty useful feature although it could just be sorted out afterwards...

@Avamander
Copy link
Collaborator

There's multiple aspects in play here that I would like to point out.

  1. If a pull request contains a feature you like then GitHub has reacts, please use those.
  2. Not being able to dismiss notifications directly does not mean they do not get replaced by newer notifications, the functionality keeps working.
  3. This is a PR that enhances an existing feature, for obvious reasons it does not get the same amount of attention as e.g. bug- or security improvements.
  4. One should have patience when there's the time of other people at play - making, reviewing and testing a PR take time, time which they might not have immediately available. If this process is something you want to speed up, there's a few avenues you can take - testing out the PR, submitting small patches are just a few examples.
  5. The PR has a few very valid review comments that need addressing in addition to the animation consistency.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement to an existing app/feature needs more work This PR needs more work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ability to clear notifications Ability to dismiss notifications