Implemented Drawing Activity #9394

Akshay0701 · 2021-08-10T07:43:36Z

Pull Request template

Fixes

Done

created drawing activity
extracted createInstance in a method of whiteboard
change WeakReference to AnkiActivity in whiteboard to let other activities use whiteboard view.
added drawing button in BasicImageFieldController

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration (SDK version(s), emulator or physical, etc)

Learning (optional, can help others)

Describe the research stage

Links to blog posts, patterns, libraries or addons used to solve this problem

Checklist

Please, go through these checks before submitting the PR.

You have not changed whitespace unnecessarily (it makes diffs hard to read)
You have a descriptive commit message with a short title (first line, max 50 chars).
Your code follows the style of the project (e.g. never omit braces in if statements)
You have commented your code, particularly in hard-to-understand areas
You have performed a self-review of your own code
UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
UI Changes: You have tested your change using the Google Accessibility Scanner

Akshay0701 · 2021-08-10T07:44:32Z

I think commits should split,
could please you suggest me order for small commits?

commit 1: created drawing activity
commit 2: extracted createInstance in a method of whiteboard
commit 3: change WeakReference to AnkiActivity in whiteboard to let other activities use whiteboard view.
commit 4: added drawing button in BasicImageFieldController

will this okay?

AnkiDroid/src/main/java/com/ichi2/anki/Whiteboard.java

AnkiDroid/src/main/java/com/ichi2/anki/DrawingActivity.kt

AnkiDroid/src/main/java/com/ichi2/anki/Whiteboard.java

AnkiDroid/src/main/java/com/ichi2/anki/multimediacard/fields/BasicImageFieldController.java

david-allison · 2021-08-10T17:33:20Z

I think these are good (only changed the order)

extracted createInstance in a method of whiteboard
change WeakReference to AnkiActivity in whiteboard to let other activities use whiteboard view.
created drawing activity
added drawing button in BasicImageFieldController

AnkiDroid/src/main/java/com/ichi2/compat/Compat.java

david-allison

This looks great! One comment regarding the cast to AbstractFlashcardViewer, the rest are nitpicks

Blocked by #9409

david-allison · 2021-08-17T02:25:53Z

AnkiDroid/src/main/java/com/ichi2/anki/DrawingActivity.kt

+            resultData.putExtra(EXTRA_RESULT_WHITEBOARD, savedWhiteboardFileName)
+            setResult(RESULT_OK, resultData)
+        } catch (e: FileNotFoundException) {
+            Timber.e(e)


Suggested change

Timber.e(e)

Timber.w(e)

AnkiDroid/src/main/java/com/ichi2/anki/Whiteboard.java

david-allison · 2021-08-17T02:28:02Z

AnkiDroid/src/main/java/com/ichi2/anki/multimediacard/fields/BasicImageFieldController.java

+
+    private void handleDrawingResult(Uri imageUri) {
+        if (imageUri == null) {
+            Timber.e("handleDrawingResult() no image path provided");


Suggested change

Timber.e("handleDrawingResult() no image path provided");

Timber.w("handleDrawingResult() no image path provided");

david-allison · 2021-08-17T02:28:24Z

AnkiDroid/src/main/java/com/ichi2/anki/multimediacard/fields/BasicImageFieldController.java

+
+            Timber.i("handleDrawingResult() Decoded image: '%s'", drewImagePath);
+        } catch (IOException e) {
+            Timber.e("");


Suggested change

Timber.e("");

Timber.w(e);

david-allison · 2021-08-17T02:29:01Z

AnkiDroid/src/main/res/values/constants.xml

@@ -22,6 +22,7 @@
    -->
    <!-- App name -->
    <string name="app_name" comment="The name of the app">AnkiDroid</string>
+    <string name="drawing">Drawing</string>


Let's move this to 01-core

david-allison

Much nicer!

AnkiDroid/src/main/java/com/ichi2/anki/Whiteboard.java

david-allison · 2021-08-17T06:40:56Z

AnkiDroid/src/main/java/com/ichi2/anki/WhiteboardMethods.java

+
+package com.ichi2.anki;
+
+public interface WhiteboardMethods {


Could do with a better name and javadocs, but this is exactly what we need code-wise

/** * WhiteboardMethods interface will provide abstract methods to override by abstractflascardviewer class * for handling multi touch while whiteboard is on. */ public interface WhiteboardMultiTouchMethods {

see this, is this doc correct?

it would be great if you could suggest me some names...

Sounds great!

Slight change to the documentation. You want to try to avoid referencing a concrete implementation for an interface (although it's often useful to provide an example).

/** * Provides callbacks for multi touch handling on a whiteboard */ public interface WhiteboardMultiTouchMethods {

AnkiDroid/src/main/java/com/ichi2/anki/Whiteboard.java

david-allison · 2021-08-18T21:16:29Z

unblocked, awaiting de-conflict

Akshay0701 · 2021-08-20T17:32:48Z

sorry, I didn't get much time last week, but now I am free so let's get this done...

david-allison

Two very minor things, but LGTM!

david-allison · 2021-08-21T00:06:50Z

AnkiDroid/src/main/java/com/ichi2/anki/WhiteboardMethods.java

+
+package com.ichi2.anki;
+
+public interface WhiteboardMethods {


Sounds great!

Slight change to the documentation. You want to try to avoid referencing a concrete implementation for an interface (although it's often useful to provide an example).

/** * Provides callbacks for multi touch handling on a whiteboard */ public interface WhiteboardMultiTouchMethods {

AnkiDroid/src/main/res/menu/drawing_menu.xml

Akshay0701 · 2021-08-22T08:41:45Z

@mikehardy what do you say,

do we have a userbase of API 21 or below?

should we rescale the image before saving it(in API 21 or below)?

mikehardy · 2021-08-22T12:52:53Z

Yes, minsdkversion is still 21, won't move for a while, the whole android ecosystem just moved up to it in fact. Probably a year or more before it goes higher. Still tens of thousands of users

david-allison

Discussed with @Akshay0701, moving the issue of the image size to #9439

Could we find out which API version this issue is resolved with, and limit the feature to API versions that are working correctly (likely via @Requires() with a comment, and make the button gone if it's not available).

Note that since we haven't diagnosed the issue, it may be screen size related, in which case we should get it fixed before getting this feature in.

AnkiDroid/src/main/java/com/ichi2/anki/multimediacard/fields/BasicImageFieldController.java

Akshay0701 · 2021-08-23T12:06:03Z

Note that since we haven't diagnosed the issue, it may be screen size related, in which case we should get it fixed before getting this feature in.

David, I am sure this issue is not related to screen size, because
I tried pixel 1(1080 x 1920: 420 dpi) with API version 21,22,23 and this issue occurred at all times.
and I tried again with the same devices pixel 1 with API version 24 and the issue didn't occur there.

david-allison · 2021-08-23T13:40:15Z

Could you add a comment on the API restriction listing the issue number, then good to go!

Akshay0701 · 2021-08-23T15:37:59Z

i think i already did comment with issue number

david-allison

Ah, was missed in the GitHub preview. LGTM!

mikehardy · 2021-08-24T19:02:23Z

AnkiDroid/src/main/java/com/ichi2/anki/multimediacard/fields/BasicImageFieldController.java

+    public String getPathFromUri(Uri uri) throws IOException {
+        String filename = ContentResolverUtil.getFileName(mActivity.getContentResolver(), uri);
+        InputStream fd =  mActivity.getContentResolver().openInputStream(uri);
+        Map.Entry<String, String> fileNameAndExtension = FileUtil.getFileNameAndExtension(filename);
+        filename = fileNameAndExtension.getKey();
+        File clipCopy = File.createTempFile(filename, fileNameAndExtension.getValue());
+        CompatHelper.getCompat().copyFile(fd, clipCopy.getAbsolutePath());
+        return clipCopy.getAbsolutePath();
+    }


This to me is return internalizeUri(uri).getAbsolutePath(), which is defined just below this method
In fact, there is a proliferation of methods like this:

MediaRegistration::loadImageIntoCollection does it

FileUtil::internalizeUri does it

Why are we duplicating the process of "take a URI, copy the contents of it into our local area and give me a file" in so many places? 🤔

I may be missing something, but I would love for this logic to be in one spot

Great review, Thank you for letting me know about the internalizeUri method, I was unaware of this method,
Done now.

mikehardy

I like this a lot - it appears to me that all the things required for it to actually work, are working. Which is awesome. My only concern is that I've seen this URI-->File logic in so many places and it looks like this is adding another? Can we avoid it at all?

mikehardy

Nice! thanks for the shift to internalizeUri

Akshay0701 mentioned this pull request Aug 10, 2021

WYSIWYG Editor #9048

Closed

14 tasks

david-allison reviewed Aug 10, 2021

View reviewed changes

Akshay0701 force-pushed the drawing_activity branch 5 times, most recently from 218b03e to c94a5f1 Compare August 12, 2021 18:03

Akshay0701 requested a review from david-allison August 12, 2021 18:32

farjadilyas reviewed Aug 14, 2021

View reviewed changes

AnkiDroid/src/main/java/com/ichi2/compat/Compat.java Outdated Show resolved Hide resolved

david-allison reviewed Aug 17, 2021

View reviewed changes

david-allison added Blocked by dependency Currently blocked by some other dependent / related change Needs Author Reply Waiting for a reply from the original author labels Aug 17, 2021

Akshay0701 force-pushed the drawing_activity branch from c94a5f1 to e270e3b Compare August 17, 2021 06:36

david-allison reviewed Aug 17, 2021

View reviewed changes

AnkiDroid/src/main/java/com/ichi2/anki/Whiteboard.java Outdated Show resolved Hide resolved

david-allison removed the Blocked by dependency Currently blocked by some other dependent / related change label Aug 18, 2021

Extracted createInstance in a method of whiteboard

3c21be3

Akshay0701 force-pushed the drawing_activity branch from e270e3b to 667b9a5 Compare August 20, 2021 16:38

david-allison self-assigned this Aug 20, 2021

david-allison added Needs Review and removed Needs Author Reply Waiting for a reply from the original author labels Aug 20, 2021

Akshay0701 force-pushed the drawing_activity branch 2 times, most recently from 52c2f0d to c8c9da8 Compare August 20, 2021 18:48

david-allison reviewed Aug 21, 2021

View reviewed changes

david-allison removed their assignment Aug 21, 2021

david-allison removed the Needs Review label Aug 21, 2021

Akshay0701 mentioned this pull request Aug 23, 2021

On API<=23 emulator the image was lager than screen viewport #9439

Closed

Akshay0701 requested a review from david-allison August 23, 2021 05:58

david-allison requested changes Aug 23, 2021

View reviewed changes

Akshay0701 force-pushed the drawing_activity branch 2 times, most recently from cb61578 to 27c7043 Compare August 23, 2021 11:52

Akshay0701 commented Aug 23, 2021

View reviewed changes

AnkiDroid/src/main/java/com/ichi2/anki/multimediacard/fields/BasicImageFieldController.java Show resolved Hide resolved

david-allison approved these changes Aug 24, 2021

View reviewed changes

david-allison added Needs Second Approval Has one approval, one more approval to merge and removed Needs Author Reply Waiting for a reply from the original author labels Aug 24, 2021

mikehardy reviewed Aug 24, 2021

View reviewed changes

mikehardy requested changes Aug 24, 2021

View reviewed changes

mikehardy added the Needs Author Reply Waiting for a reply from the original author label Aug 24, 2021

Akshay0701 force-pushed the drawing_activity branch 3 times, most recently from 1adb1c1 to 177fe70 Compare August 25, 2021 12:06

Added drawing button in BasicImageFieldController

199df53

Akshay0701 force-pushed the drawing_activity branch from 177fe70 to 199df53 Compare August 25, 2021 13:15

mikehardy approved these changes Aug 25, 2021

View reviewed changes

mikehardy removed Needs Author Reply Waiting for a reply from the original author Needs Second Approval Has one approval, one more approval to merge labels Aug 25, 2021

mikehardy added this to the 2.16 release milestone Aug 25, 2021

mikehardy added the Strings label Aug 25, 2021

mikehardy merged commit 4587fed into ankidroid:master Aug 25, 2021

david-allison mentioned this pull request Nov 18, 2021

Add new card with drawboard #1633

Closed

kmicklas mentioned this pull request Apr 21, 2024

Undo stroke in drawing editor #16241

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented Drawing Activity #9394

Implemented Drawing Activity #9394

Akshay0701 commented Aug 10, 2021 •

edited by david-allison

Akshay0701 commented Aug 10, 2021 •

edited

david-allison commented Aug 10, 2021

david-allison left a comment

david-allison Aug 17, 2021

david-allison Aug 17, 2021

david-allison Aug 17, 2021

david-allison Aug 17, 2021

david-allison left a comment

david-allison Aug 17, 2021

Akshay0701 Aug 20, 2021 •

edited

david-allison Aug 21, 2021

david-allison commented Aug 18, 2021

Akshay0701 commented Aug 20, 2021

david-allison left a comment

david-allison Aug 21, 2021

Akshay0701 commented Aug 22, 2021 •

edited

mikehardy commented Aug 22, 2021

david-allison left a comment •

edited

Akshay0701 commented Aug 23, 2021 •

edited

david-allison commented Aug 23, 2021

Akshay0701 commented Aug 23, 2021

david-allison left a comment

mikehardy Aug 24, 2021

Akshay0701 Aug 25, 2021

mikehardy left a comment

mikehardy left a comment

	Timber.e("handleDrawingResult() no image path provided");
	Timber.w("handleDrawingResult() no image path provided");

Implemented Drawing Activity #9394

Implemented Drawing Activity #9394

Conversation

Akshay0701 commented Aug 10, 2021 • edited by david-allison

Pull Request template

Fixes

Done

How Has This Been Tested?

Learning (optional, can help others)

Checklist

Akshay0701 commented Aug 10, 2021 • edited

david-allison commented Aug 10, 2021

david-allison left a 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

Choose a reason for hiding this comment

david-allison left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Akshay0701 Aug 20, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-allison commented Aug 18, 2021

Akshay0701 commented Aug 20, 2021

david-allison left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Akshay0701 commented Aug 22, 2021 • edited

mikehardy commented Aug 22, 2021

david-allison left a comment • edited

Choose a reason for hiding this comment

Akshay0701 commented Aug 23, 2021 • edited

david-allison commented Aug 23, 2021

Akshay0701 commented Aug 23, 2021

david-allison left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikehardy left a comment

Choose a reason for hiding this comment

mikehardy left a comment

Choose a reason for hiding this comment

Akshay0701 commented Aug 10, 2021 •

edited by david-allison

Akshay0701 commented Aug 10, 2021 •

edited

Akshay0701 Aug 20, 2021 •

edited

Akshay0701 commented Aug 22, 2021 •

edited

david-allison left a comment •

edited

Akshay0701 commented Aug 23, 2021 •

edited