-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Live Photo #12139
Live Photo #12139
Conversation
Signed-off-by: alperozturk <[email protected]>
Signed-off-by: alperozturk <[email protected]>
Signed-off-by: alperozturk <[email protected]>
Signed-off-by: alperozturk <[email protected]>
Signed-off-by: alperozturk <[email protected]>
Signed-off-by: alperozturk <[email protected]>
Signed-off-by: alperozturk <[email protected]>
Signed-off-by: alperozturk <[email protected]>
@alperozturk96 the icon is: https://developer.apple.com/live-photos/ So I would just use that, no? See https://thenounproject.com/browse/icons/term/live-photo/ for svg resources, maybe @jancborchardt or @nimishavijay can pick one? :) |
@AndyScherzinger I agree that icon more suitable. @tobiasKaminsky suggested that icon. If it's okay for all of us I can change. |
This icon seems to be the closest to Apple’s live photo icon: https://thenounproject.com/icon/play-live-photo-710577/ |
Signed-off-by: alperozturk <[email protected]>
Signed-off-by: alperozturk <[email protected]>
Icons updated @AndyScherzinger @jancborchardt |
@alperozturk96 sorry, the actual icon Apple uses is https://thenounproject.com/icon/live-photo-710576/, not the one with the play icon, my mistake. Also, the iOS Photos app only shows the "Live" indicator when in the photo detail view, not overlaid on each photo as this makes the grid view very busy. I would say we should do it similarly to keep it clean and focused on the photos. |
android:paddingStart="@dimen/standard_half_padding" | ||
android:paddingEnd="@dimen/alternate_padding" | ||
android:src="@drawable/ic_checkbox_blank_outline" /> | ||
|
||
<ImageView | ||
android:id="@+id/overflow_menu" | ||
android:layout_width="48dp" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
48dp is needed for Accessibility, icon hitboxes need to be at least 48dp in size
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a dimens value for that also 👍
@jancborchardt i.e. amazon is showing it, which I think makes sense, since we are not the Apple Image viewer but a general EFSS in this case, so if we then hide the mov file in these file lists than we might want to indicate a live photo as such, no? |
Signed-off-by: alperozturk <[email protected]>
Signed-off-by: alperozturk <[email protected]>
Signed-off-by: alperozturk <[email protected]>
For the overlay in the grid view this definitely needs a background container to "punch out" the icon, else it is extremely hard to spot. So I would suggest just using the background color (surface), semi-transparent might look strange, on the other hand the share icon has the same issue too. |
Ok, then if we show it I would say
|
@jancborchardt can maybe @marcoambrosini check how Apple displays it on dark mode (since I only have Android) |
@AndyScherzinger is this what you need? |
Signed-off-by: alperozturk <[email protected]>
perfect, yes, thanks @marcoambrosini 👍 |
Signed-off-by: alperozturk <[email protected]>
Signed-off-by: alperozturk <[email protected]>
@@ -35,7 +35,7 @@ | |||
*/ | |||
public class ProviderMeta { | |||
public static final String DB_NAME = "filelist"; | |||
public static final int DB_VERSION = 73; | |||
public static final int DB_VERSION = 75; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DB version bump by 2? Also where is the new json schema for DB?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was testing. Multiple times I came across with conflict and I was increasing one by one whenever I got that issue. I remember first one was 74 then I put 75 nevertheless after complete all implementation I am going to check all changes. Thanks for the early notice.
Signed-off-by: tobiasKaminsky <[email protected]>
Hi, I noticed some bugs.
Record_2023-12-06-12-04-08.mp4
Record_2023-12-06-12-04-31.mp4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix described bugs
Could you share crash report? |
For 2.: For 3.: I get "com.owncloud.android.ui.preview.PreviewImageActivity.onOptionsItemSelected(PreviewImageActivity.java:226)" a hundred time befor app actually crashes for some reason (or logger bug?) |
This bug fixed in this PR already |
Signed-off-by: alperozturk <[email protected]>
Signed-off-by: github-actions <[email protected]>
Signed-off-by: alperozturk <[email protected]>
Signed-off-by: alperozturk <[email protected]>
Signed-off-by: alperozturk <[email protected]>
Signed-off-by: alperozturk <[email protected]>
Signed-off-by: github-actions <[email protected]>
Signed-off-by: alperozturk <[email protected]>
Signed-off-by: alperozturk <[email protected]>
Signed-off-by: alperozturk <[email protected]>
APK file: https://www.kaminsky.me/nc-dev/android-artifacts/12139.apk |
/backport to stable-3.27 |
Resolves #12124
Live Photo Indicator List View:
Live Photo Indicator Grid View:
Live Photo Indicator List View SFSymbol Alternative:
Live Photo Indicator Grid View SFSymbol Alternative:
@nextcloud/designers How does the live photo indicator look? I think the position and color can be better, but nothing come in my mind in this regard.