Add UI for image-attachment "focus" #2620

mcclure · 2022-07-16T05:29:52Z

Choose "Set focus" in the attachment menu, tap once to select focus point, tap "OK". Has been successfully tested with mastodon.social (image has two identical very tall image attachments, but in one the focus point is set near the top and in the other it is set near the bottom).

~~Currently very ugly, no visual feedback of where/whether you tapped and I think a naive user would be totally confused by what the dialog even does:~~ See new screenshots

But, it works, so should be a good starting point for a more complete attempt.

Things I think should change before submit:

~~Thumbnails in compose view should refocus on focus point~~
~~Some visual feedback of tap, a bullseye or something~~
~~I need to do more aggressive testing (changing focus points multiple times, changing caption before and after etc)~~

Things I'm 50% sure should change before submit:

~~It would be nice if you could drag a finger to move around the focus point instead of only taps working (this may require replacing chrisbanes.photoview)~~
~~Currently if you tap outside the photo view (like slightly to the right or left) the tap does not register. That's very bad (may require replacing chrisbanes.photoview)~~
Some text above the image would be good. Possibly either a full explanation of how focus points work or a link to one. It could be an unfamiliar concept.
Some sort of preview would be nice. Ideally the preview would let you choose a couple different aspect ratios like 16:9 or 1:1. Maybe that could be saved for a followup PR.
~~LATE EDIT IN: Circle thickness is a fixed 10 pixels, but in my testing (see below) this appears to be relative to the underlying image. It should be a fixed thickness relative to the screen size.~~

Things I'm not sure about at all:

~~What should the menu item say? Currently it's "Set focus". Would something like "Image focus" or "Set focus point" be better?~~
~~TODO item in ComposeViewModel.kt~~
Currently the pane is inset (like "set caption"), maybe it would be good if the pane were "full screen" (like the cropper)
LATE EDIT IN: Should there be a focus checkmark image like there is for captions?
~~LATE EDIT IN: If you zoom in, then tap, it zooms all the way back out. This is functionally kind of good but feels "janky".~~
LATE EDIT IN: Pane closes if you rotate the phone

…s. Choose "Set focus" in the attachment menu, tap once to select focus point (no visual feedback currently), tap "OK". Works in tests.

mcclure · 2022-07-17T17:06:04Z

app/src/main/java/com/keylesspalace/tusky/components/compose/ComposeViewModel.kt

@@ -324,11 +325,12 @@ class ComposeViewModel @Inject constructor(
        return combineLiveData(deletionObservable, sendFlow.asLiveData()) { _, _ -> }
    }

-    suspend fun updateDescription(localId: Int, description: String): Boolean {
+    // Updates a QueuedMedia item arbitrarily, then sends description and focus to server
+    suspend fun updateMediaItem(localId: Int, mutator: (QueuedMedia) -> QueuedMedia): Boolean {


Should this be private?

mcclure · 2022-07-17T17:10:43Z

Commit 613358e removes the code duplication in ComposeViewModel which I wanted, removes the function duplication in MastodonApi which Conny wanted, and has now been aggressively tested (IE change caption then focus, change focus then caption, change both more than once, everything is correct in the end)

connyduck · 2022-07-17T18:21:27Z

Thumbnails in compose view should refocus on focus point

Some visual feedback of tap, a bullseye or something

Currently if you tap outside the photo view (like slightly to the right or left) the tap does not register. That's very bad (may require replacing chrisbanes.photoview)

Refocusing thumbnails and visual feedback definitely, the third is not that important for a first version. Maybe also show the selected focus point on the thumbnail? How about a simple circle in primary color with theme background color as outline as indicator?

What should the menu item say? Currently it's "Set focus". Would something like "Image focus" or "Set focus point" be better?

I like "set focus point" most.

Currently the pane is inset (like "set caption"), maybe it would be good if the pane were "full screen" (like the cropper)

I think fullscreen would be better, but it isn't important.

Should there be a focus checkmark image like there is for captions?

If you want, definitely not needed for a first version

… ProgressImageView now inherits from MediaPreviewImageView.

mcclure · 2022-07-23T00:33:48Z

The attachment previews in the compose view now focus properly where appropriate. ProgressImageView now inherits from MediaPreviewImageView to accomplish this. I also discovered Attachment.Focus and replaced my uses of PointF with it.

… on code from RoundedCorners builtin from Glide

mcclure · 2022-09-03T23:25:45Z

Now have "previews" working, a focus is drawn when you tap in the focus-set dialog and in the normal message view the attachment previews center on the focus points. I styled the preview in the dialog to look as much as possible like the equivalent dialog in Mastodon.social web client (I think it looks good, though it's too bad the "center point" is always white since it's not visible on a white background. (Maybe it should be drawn as an inversion? But that might look uglier than it just sometimes not being visible)). Click for full versions:

I am going to take this PR out of draft mode because I think this is the "minimum viable" version of the feature, but it is still pretty janky and it may be worth more improvements before merging. Warning, I am not sure I can fix all of the remaining steps myself with my current knowledge of Android GUI and the codebase.

I think the remaining "should happen before putting this in a release" are:

A "Tap to select the point where crops will focus" message in the box (I can do this myself) (Not necessary if we can get dragging in?)
In the examples above, the first two images are the same size but the third is a different size. I think the circle thickness is visibly different. This should be fixed. (I can maybe figure this out on my own but at this second I don't know how to do it? The easy way would just be to scale up or down the stroke width depending on the ratio of the canvas size to the size of the displayed widget, but! I'm not sure how to get the "on-screen size" of the widget we're drawing into and I'm not sure how to tell if the Android is in "Retina mode"). (Another possible fix here would be if the gray+circle were a second widget drawn over the first instead of us actually compositing the gray+circle into the PhotoView itself each time.)
Super irritating that you can't drag to move the focus (I don't know where to start with this, it would seem to involve disabling the pan/zoom features of PhotoView)
Super irritating that you can't touch OUTSIDE the photo to move the focus to the closest point (I don't know where to start with this, PhotoView has a OnOutsidePhotoTapListener event but it only tells you there WAS a touch, not where it was: Way to get location of tap outside view? Baseflow/PhotoView#823) (One way to fix this would be to have there be a widget which is the entire width of the dialog, either by making the PhotoView wider or by placing a second widget on top of the PhotoView. But I don't know how to do that.) (This would also probably make more sense if the dialog were "full screen" as described above.)

Feedback welcomed on message text / how the focus selection looks.

mcclure

asdfadsffg i didn't mean to click this button

mcclure · 2022-09-03T23:46:16Z

app/src/main/java/com/keylesspalace/tusky/components/compose/dialog/FocusDialog.kt

+import java.security.MessageDigest
+import java.util.concurrent.locks.Lock
+
+// Private, but necessary to implement BitmapTransformation, function extracted from Glide


This file now contains rather a lot of code adapted from Glide. There was no sample code for creating a custom BitmapTransformation in their docs so I used the RoundedCorners class and the TransformationUtils.roundedCorners method as bases. getAlphaSafeBitmap and getAlphaSafeConfig are taken direct from Glide TransformationUtils (but converted to Kotlin) and the "boilerplate" methods are almost identical to the ones from RoundedCorners. I do not think this introduces license issues because (1) we are already bound to the Glide license and (2) the code taken is "minimal" in the sense that it practically could not be any different than it is.

I am going to file a bug on Glide asking them to make getAlphaSafeBitmap and getAlphaSafeConfig public methods as I do not think it is possible to implement a BitmapTransformation without them.

mcclure · 2022-09-03T23:46:48Z

app/src/main/java/com/keylesspalace/tusky/components/compose/dialog/FocusDialog.kt

+        val strokePaint = Paint(Paint.ANTI_ALIAS_FLAG)
+        strokePaint.setAntiAlias(true)
+        strokePaint.setStyle(Paint.Style.STROKE)
+        val strokeWidth = 10.0f


As described above this should not be a fixed number

mcclure · 2022-09-03T23:47:08Z

app/src/main/java/com/keylesspalace/tusky/components/compose/dialog/FocusDialog.kt

+        val result = pool[toTransform.width, toTransform.height, safeConfig]
+
+        val plainPaint = Paint()
+        val strokePaint = Paint(Paint.ANTI_ALIAS_FLAG)


Very unclear if ANTI_ALIAS_FLAG is required, I tried it both ways and I THINK it looks better with but it's hard to tell

mcclure · 2022-09-03T23:48:02Z

app/src/main/java/com/keylesspalace/tusky/components/compose/dialog/FocusDialog.kt

+        }
+
+        if (!toTransform.equals(inBitmap)) {
+            pool.put(toTransform)


I am very, VERY confused if this is desirable or not. I think it's good because this will cache the image with the circle/shadow drawn on so if the set focus dialog is opened a second time it will open faster.

… a single canvas

…editor looks and feels right

mcclure · 2022-09-05T22:22:26Z

Took in Conny's commit and I think this is mergeable? One last question:

mcclure · 2022-09-05T22:22:56Z

app/src/main/java/com/keylesspalace/tusky/components/compose/view/FocusIndicatorView.kt

+        curtainPaint.style = Paint.Style.FILL
+
+        strokePaint.setStyle(Paint.Style.STROKE)
+        strokePaint.setStrokeWidth(strokeWidth)


Will this be in display-independent pixels or do I need to add * getResources().getDisplayMetrics().density ?
EDIT: RESOLVED, it's not display-independent

Tak · 2022-09-07T07:22:19Z

Is it possible to apply the focus point to the preview in the compose dialog? (could be a followup after merging)

mcclure · 2022-09-07T15:51:09Z

Is it possible to apply the focus point to the preview in the compose dialog? (could be a followup after merging)

It does this as of 7b3fd74, there is no unique indicator that a focus is present but the preview image does focus at the appropriate point

Tak · 2022-09-07T17:07:57Z

Oh, I think I see - is it possible that the focal point is getting reversed in this scenario? Here, I'm setting the focal point in the upper left, and the preview is panning to the right edge. (This is against f4e4f8b )

…behaves on wide aspect ratio screens)

mcclure · 2022-09-19T04:11:26Z

@Tak Ahh, thank you, good catch-- the X axis was inverted but the testcase I used on the previous push only tested the Y axis...!

I have a fix which I will push tomorrow.

…nd drafts (but draft focus is lost on post)

…posted in that order

mcclure · 2022-09-19T18:56:35Z

app/src/main/java/com/keylesspalace/tusky/components/compose/ComposeActivity.kt

@@ -169,6 +170,7 @@ class ComposeActivity :
                        uriNew,
                        size,
                        itemOld.description,
+                        null, // Intentionally reset focus when cropping


Is this the best approach? We could do something weird where we try to calculate where/whether the focal point is within the cropped area, but this feels like second guessing the user so I just reset.

mcclure · 2022-09-19T18:58:52Z

app/src/main/java/com/keylesspalace/tusky/components/compose/ComposeViewModel.kt

@@ -185,7 +187,7 @@ class ComposeViewModel @Inject constructor(
        return mediaItem
    }

-    private fun addUploadedMedia(id: String, type: QueuedMedia.Type, uri: Uri, description: String?) {
+    private fun addUploadedMedia(id: String, type: QueuedMedia.Type, uri: Uri, description: String?, focus: Attachment.Focus?) {


Long term observation: Adding focus to "travel along with" description when attachments hibernate/resurrect required adding plumbing in a LOT of places. It might make more sense to make a "Metadata" struct like the one already in Attachment and pass that around instead of description (although not Attachment.Meta itself, as that includes "duration", a metadata returned from the server but not tracked by us when composing attachments), in case later there is some third thing that we track per attachment (a low res version, I don't know)

mcclure · 2022-09-19T19:00:45Z

app/src/main/java/com/keylesspalace/tusky/components/drafts/DraftMediaAdapter.kt

                holder.imageView.setImageResource(R.drawable.ic_music_box_preview_24dp)
            } else {
-                Glide.with(holder.itemView.context)


Long term observation: This clause has (and had before I started changing it) significant code duplication VS MediaPreviewAdapter. Maybe it's worth a refactor here

mcclure · 2022-09-19T19:13:50Z

New changes:

Fix x flipping bug
Drafts, scheduled tweets, . Focus properly focuses in the draft view preview.
Crop window narrows itself vertically so it no longer has large amounts of empty space. Note, this ONLY affects vertical width, not horizontal width, horizontal width seems fine already.

Will post some screenshots of the new window sizing after I fix merging and do some exhaustive tests.

mcclure · 2022-09-19T20:08:56Z

How the new dialog sizes look-- they are sized to the image plus a little bit of room to draw the circle:

The window does NOT narrow horizontally if the image is wider than it is tall, I think that looks fine as is.

Landscape:

mcclure · 2022-09-19T20:16:39Z

I've tested this version and I think it is a candidate for merging. The only remaining issues/questions:

Window disappears when the phone autorotates the screen. (Caption window now resizes itself properly.)
Should explanation text go above the image or is it obvious enough?

connyduck · 2022-09-20T19:18:48Z

It works so well now, I love it. Thank you very much.

Window disappears when the phone autorotates the screen. (Caption window now resizes itself properly.)

We had the same problem for the caption dialog, see here how it was fixed. It is no big deal though.

Should explanation text go above the image or is it obvious enough?

You mean the "Click or drag the circle on the preview to choose the focal point which will always be in view on all thumbnails." that the web interface has? Would be nice, yes but it is also no blocker for merging this.

Android studio gives me a lot of code style warnings, can you fix those please? It is mostly property access syntax vs getter/setter and Kotlin functions instead of java Math ones.

mcclure · 2022-09-20T22:29:16Z

I'm finding it a little hard to get full lists of Android Studio warnings, but I think those last two commits fix all the warnings in code the PR touches.

I left some warnings as-is:

Many claims that "Tusky" is a typo 🙄
Ignored an if-null foldable in FocusIndicatorView because the suggested alternative would be pretty ugly
Ignored "'SOFT_INPUT_ADJUST_RESIZE: Int' is deprecated. Deprecated in Java" in FocusDialog; I copied this code from CaptionDialog and I don't at present know either what it does or what to replace it with

I also explicitly @SuppressLint'd and commented an accessibility warning because FocusIndicatorView implements a touch event but not the "click" event from the screen reader and there's legitimately nothing that can be done with the screen reader click. FocusIndicatorView interaction is not meaningful without an x,y position. If there's something better I should do here instead (implement performClick but return false because we didn't handle it? hide focus select from screen readers completely? or is there actually a way to make this pane accessible?) I'd be happy to do that instead.

I wouldn't mind trying to replicate the CaptionDialog refactor at some later point, but I'd prefer to merge this PR first so that the PR does not develop conflicts with the develop branch.

connyduck · 2022-09-21T18:01:40Z

I also explicitly @SuppressLint'd and commented an accessibility warning because FocusIndicatorView implements a touch event but not the "click" event from the screen reader and there's legitimately nothing that can be done with the screen reader click. FocusIndicatorView interaction is not meaningful without an x,y position. If there's something better I should do here instead (implement performClick but return false because we didn't handle it? hide focus select from screen readers completely? or is there actually a way to make this pane accessible?) I'd be happy to do that instead.

I'm not sure. I'd leave it for now and wait for feedback from people using Tusky with screenreaders.

mcclure added 2 commits July 16, 2022 01:01

Attempt-zero implementation of a "focus" feature for image attachment…

f3bde24

…s. Choose "Set focus" in the attachment menu, tap once to select focus point (no visual feedback currently), tap "OK". Works in tests.

Remove code duplication between 'update description' and 'update focus'

613358e

mcclure commented Jul 17, 2022

View reviewed changes

mcclure added 4 commits July 17, 2022 21:27

Fix ktlint/bitrise failures

7d86015

Make updateMediaItem private

c283a4c

When focus is set on a post attachment the preview focuses correctly.…

7b3fd74

… ProgressImageView now inherits from MediaPreviewImageView.

Replace use of PointF for Focus where focus is represented, fix ktlint

02b5e68

mcclure added 5 commits July 22, 2022 23:22

Substitute 'focus' for 'focus point' in strings

84926d9

First attempt draw focus point. Only updates on initial load. Modeled…

a439ec5

… on code from RoundedCorners builtin from Glide

Redraw focus after each tap

5e920da

Dark curtain where focus isn't (now looks like mastosoc)

09ab141

Correct ktlint for FocusDialog

8f08b1b

mcclure marked this pull request as ready for review September 3, 2022 23:38

mcclure commented Sep 3, 2022

View reviewed changes

connyduck and others added 5 commits September 5, 2022 12:56

draft: switch to overlay for focus indicator

d345e62

Draw focus circle, but ImageView and FocusIndicatorView seem to share…

c89cdcf

… a single canvas

Switch focus circle to path approach

3437fa8

Correctly scale, save and load focuses. Clamp to visible area. Focus …

480b07b

…editor looks and feels right

ktlint fixes and comments

0e6c942

mcclure commented Sep 5, 2022

View reviewed changes

Focus indicator drawing should use device-independent pixels

f4e4f8b

mcclure added 2 commits September 18, 2022 19:26

Shrink focus window when it gets unattractively tall (no linting, mis…

d929b98

…behaves on wide aspect ratio screens)

Correct max-height behavior for screens in landscape mode

dab6381

mcclure added 4 commits September 19, 2022 10:50

Focus attachment result is are flipped on x axis; fix this

af5c995

Correctly thread focus through on scheduled posts, redrafted posts, a…

829c151

…nd drafts (but draft focus is lost on post)

More focus ktlint fixes

cde5d5e

Fix specific case where a draft is given a focus, then deleted, then …

a93369b

…posted in that order

mcclure commented Sep 19, 2022

View reviewed changes

Fix accidental file change in focus PR

400683d

mcclure added 2 commits September 19, 2022 15:27

Remerge develop with focus PR

a81ad1c

ktLint fix

829a0ed

mcclure added 2 commits September 20, 2022 17:51

Fix property style warnings in focus

fc771f3

Fix remaining style warnings from focus PR

33d5745

connyduck approved these changes Sep 21, 2022

View reviewed changes

connyduck merged commit 7684f06 into tuskyapp:develop Sep 21, 2022

Tak mentioned this pull request Feb 14, 2023

Support the focal point feature introduced in Mastodon 2.3 #648

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add UI for image-attachment "focus" #2620

Add UI for image-attachment "focus" #2620

mcclure commented Jul 16, 2022 •

edited

Loading

mcclure Jul 17, 2022

connyduck Jul 17, 2022

mcclure commented Jul 17, 2022 •

edited

Loading

connyduck commented Jul 17, 2022

mcclure commented Jul 23, 2022

mcclure commented Sep 3, 2022 •

edited

Loading

mcclure left a comment •

edited

Loading

mcclure Sep 3, 2022

mcclure Sep 3, 2022

mcclure Sep 3, 2022

mcclure Sep 3, 2022

mcclure commented Sep 5, 2022

mcclure Sep 5, 2022 •

edited

Loading

Tak commented Sep 7, 2022

mcclure commented Sep 7, 2022

Tak commented Sep 7, 2022 •

edited

Loading

mcclure commented Sep 19, 2022

mcclure Sep 19, 2022

mcclure Sep 19, 2022

mcclure Sep 19, 2022

mcclure commented Sep 19, 2022

mcclure commented Sep 19, 2022 •

edited

Loading

mcclure commented Sep 19, 2022 •

edited

Loading

connyduck commented Sep 20, 2022

mcclure commented Sep 20, 2022

connyduck commented Sep 21, 2022

Add UI for image-attachment "focus" #2620

Add UI for image-attachment "focus" #2620

Conversation

mcclure commented Jul 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcclure commented Jul 17, 2022 • edited Loading

connyduck commented Jul 17, 2022

mcclure commented Jul 23, 2022

mcclure commented Sep 3, 2022 • edited Loading

mcclure left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcclure commented Sep 5, 2022

mcclure Sep 5, 2022 • edited Loading

Choose a reason for hiding this comment

Tak commented Sep 7, 2022

mcclure commented Sep 7, 2022

Tak commented Sep 7, 2022 • edited Loading

mcclure commented Sep 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcclure commented Sep 19, 2022

mcclure commented Sep 19, 2022 • edited Loading

mcclure commented Sep 19, 2022 • edited Loading

connyduck commented Sep 20, 2022

mcclure commented Sep 20, 2022

connyduck commented Sep 21, 2022

mcclure commented Jul 16, 2022 •

edited

Loading

mcclure commented Jul 17, 2022 •

edited

Loading

mcclure commented Sep 3, 2022 •

edited

Loading

mcclure left a comment •

edited

Loading

mcclure Sep 5, 2022 •

edited

Loading

Tak commented Sep 7, 2022 •

edited

Loading

mcclure commented Sep 19, 2022 •

edited

Loading

mcclure commented Sep 19, 2022 •

edited

Loading