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

Mobile: Resolves #9377: Don't attach empty drawings when a user exits without saving #9386

Conversation

personalizedrefrigerator
Copy link
Collaborator

@personalizedrefrigerator personalizedrefrigerator commented Nov 26, 2023

Summary

Previously, clicking "Draw picture"

  1. created a new, empty image resource,
  2. attached that resource to the current note, then
  3. opened the resource in the image editor.

This pull request changes that behavior to the following:

  1. Opens the image editor with no resource
  2. On the first save, creates and attaches a new image resource

This prevents empty drawings from being attached to a note when the user clicks "discard changes" or "close" without ever saving the drawing.

Note

This is a partial fix that only applies to the note viewer.
The CodeMirror editor requires a reference to insert text (used for attaching the drawing). This reference is not available while the image editor is visible. This pull request thus only applies the fix in the case where the note viewer is open.

Note.tsx will likely require refactoring for a more complete fix. (For example by making the CodeMirror editor watch its initialText prop for changes or to send the insertText command to the editor by changing a prop).

Resolves #9377.

Testing plan

  1. Click "Draw picture" from the note actions menu
  2. Click "close"
  3. Verify that no new drawing has been attached to the note
  4. Click "Draw picture"
  5. Draw something and click save
  6. Draw something else.
  7. Click "close", then "save changes"
  8. Verify that the drawing has been attached with changes saved
  9. Edit the drawing
  10. Make a change
  11. Close and save
  12. Verify that the changes have been saved
  13. Android only:
    1. Click "draw picture"
    2. Draw something
    3. Click "save"
    4. Forward a port from the Android device to the computer
    5. Open chrome://inspect
    6. Open the developer tools for the WebView
    7. Type location.reload() in the console
    8. Verify that the saved drawing is still visible

This has been successfully tested on an Android 12 emulator.

@personalizedrefrigerator personalizedrefrigerator changed the base branch from dev to release-2.13 November 27, 2023 13:56
@personalizedrefrigerator personalizedrefrigerator force-pushed the pr/mobile-attach-image-only-after-first-save branch from b517321 to f381c3a Compare November 27, 2023 14:04
@personalizedrefrigerator personalizedrefrigerator force-pushed the pr/mobile-attach-image-only-after-first-save branch from f381c3a to b546b95 Compare November 27, 2023 14:04
@personalizedrefrigerator personalizedrefrigerator marked this pull request as draft November 27, 2023 14:40
@personalizedrefrigerator
Copy link
Collaborator Author

personalizedrefrigerator commented Nov 27, 2023

I'm converting this to a draft — it causes a regression when attaching drawings from the CodeMirror editor.

Currently, when the image editor is open, the reference to the CodeMirror editor is outdated. As such, calling .insertText to insert the new resource doesn't work.

Edit: Planned (partial) solution: Only delay creating a resource when the note viewer is open. In the note editor, insert the resource immediately (as was done before this pull request).

A more complete solution will likely require refactoring, for example, by having the note editor watch for changes to the initialText prop (like the desktop editor). Such a change, however, could cause subtle regressions and should not be introduced during a feature freeze.

@personalizedrefrigerator personalizedrefrigerator marked this pull request as ready for review November 27, 2023 18:17
@laurent22 laurent22 merged commit f0a1b41 into laurent22:release-2.13 Nov 27, 2023
10 checks passed
laurent22 pushed a commit that referenced this pull request Nov 30, 2023
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.

JS-Draw images that are empty are invisible in light theme
2 participants