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

Gallery - context menu - 2nd Attempt #985

Merged
merged 25 commits into from
Jan 4, 2021
Merged

Gallery - context menu - 2nd Attempt #985

merged 25 commits into from
Jan 4, 2021

Conversation

nayooti
Copy link
Contributor

@nayooti nayooti commented Nov 4, 2020

closes #944
closes #891

Since MessageKit-files were deleted I was not able to rebase properly, so I had to rebase "manually" using this branch.

This PR:

  • context menu now displays (and plays) videos like on Telegram.
  • context menu works on different device rotations
  • context menu closes on device rotation -> this is intended native behaviour
  • context menu for documents gallery in iOS 13
  • contextMenuProvider to generate legacy and new styled context menus
  • context menu provides show in chat that redirects to the message in chat view
  • same menu applied to DocumentGalleryController (modern context menu uses default preview)

Gallery did not look nice in landscape on devices with notch. I did a quick fix in this PR too.

To test:

  • use videos+images in landscape and portrait
  • try device in portrait and landscape

replaces #952

@cyBerta
Copy link
Contributor

cyBerta commented Nov 5, 2020

I just had a short check on my phone. It looks great, video works too. What is missing is support for gif files. At least the first image of an gif should be shown, if previewing gifs is not possible. Currently there's a white image shown.

Copy link
Contributor

@cyBerta cyBerta left a comment

Choose a reason for hiding this comment

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

see my comment

@nayooti nayooti changed the title Gallery - context menue - 2nd Attempt Gallery - context menu - 2nd Attempt Nov 12, 2020
@nayooti nayooti requested a review from cyBerta November 12, 2020 09:47
@r10s
Copy link
Member

r10s commented Nov 12, 2020

great!

some ui things:

  • the delete-button should be red (see other apps)
  • once tapped, there should be a confirmation (also see other apps)

to the general understanding:

  • the menu is a normal ViewController, or? os maybe call it ContextMenuViewController.swift to point out that it is just this. i was a bit confused what a MenuController is :)
  • all in all this is some code and complexity - if we want to get the menu to the document view and - even more interestingly - to the normal chatview, we can resuse ContextMenuViewController.swift? (of course, we would need to add renderings for normal messages and files then)

@nayooti
Copy link
Contributor Author

nayooti commented Nov 12, 2020

MenuController is from Apple to show the old context menu (iOS 12 and lower)
`ContextMenuViewController' to show new context menu (iOS 13 and after).

ContextMenuViewController is embedded into the iOS13-context menu and used to display different viewTypes. It can be easily be reused and extended. For example to be used in ChatViewController, but obviously this needs to be extended for different MessageTypes.

The complexity is only that we have to cover both context-menu-styles as long we support iOS12 and lower.
After all, the complexity isn't too bad IMO.

@nayooti
Copy link
Contributor Author

nayooti commented Nov 12, 2020

@r10s I could not find any suitable confirm-delete-string. Any idea?

@nayooti nayooti requested a review from r10s November 12, 2020 15:36
@r10s
Copy link
Member

r10s commented Nov 13, 2020

we're getting closer :)

some more things, for ios 11, 12:

  • when the old-style context menu is opened, a tap outside the context menu should just close it. however, then this tap goes to the image, it also enlarges the image. this enlargement should require a second tap.

you can test this eg. in "iPhone 8 (iOS 12.2)" emulator

things for ios 13+:

  • when the new-style context menu is opened, a tap outside the context menu should close it. this is correct. a tap inside, on the preview image should directly enlarge the image. please check the normal ios gallery to get the idea.

  • the delete confirmation is okayish, however, the ios gallery stays in the context-menu for the confirmation. this is no blocker, however, if this is easily doable by only setting a flag or so, we should just do it the same way. if it adds complexity, however, we should postpone it.

@nayooti nayooti force-pushed the delete-galler-item-2 branch from b61b745 to 6994533 Compare November 27, 2020 14:20
@nayooti nayooti force-pushed the delete-galler-item-2 branch from 99212b2 to 5fb89cd Compare December 14, 2020 17:04
@nayooti
Copy link
Contributor Author

nayooti commented Dec 14, 2020

@r10s Ok, I am done for now.

@cyBerta
Copy link
Contributor

cyBerta commented Dec 23, 2020

There's an unresolved merge conflict and unfortunately also new merge conflcts with latest master that need to be resolved, too.

@cyBerta cyBerta self-assigned this Jan 4, 2021
@cyBerta cyBerta force-pushed the delete-galler-item-2 branch from aa0bbed to e3762a3 Compare January 4, 2021 14:43
Copy link
Contributor

@cyBerta cyBerta left a comment

Choose a reason for hiding this comment

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

lgtm, I fixed the merge conflict and removed the controls from the video preview, because a tap on any item within the preview will open the preview controller, so the controls can never be triggered. It's now the same UI as in the iOS Gallery wrt. video previews.

@cyBerta cyBerta merged commit dd67965 into master Jan 4, 2021
@cyBerta cyBerta deleted the delete-galler-item-2 branch January 4, 2021 15:51
@cyBerta cyBerta mentioned this pull request Jan 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add option to delete image from gallery add context menu for gallery and documents
3 participants