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.

Sign up for GitHub

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

Jump to bottom

feat: Feature/image element #3424

Closed
wants to merge 45 commits into from
Closed

feat: Feature/image element #3424

wants to merge 45 commits into from

Conversation

heitara
Copy link
Contributor

@heitara heitara commented Apr 8, 2021

Hello,

This branch is based on upload-images, but I've applied all the changes to the most recent version of master. I've update the load image logic to fit a bit better. I've added a centralized image cache.

A few things are not working as expected and I need some directions to finalize those.

  1. Upon initial creation of an image element the image is not rendered every time. This is a race condition problem, which can be solved with a refresh of that very item. I'm not sure how this should be done properly. (For example, if the window is resized the image element renders properly, or it's copy-and-paste-d, or resized)
  2. The cache is constantly growing. I envision that an image should be removed from the cache once there is no image element using that image. At the moment this should happen in two actions - delete or cut.
  3. In shared sessions the images are not synced. This should be implemented after the first two are resolved.
  4. No tests, yet!

Any feedback, requests or updates are welcome.

@vercel
Copy link

vercel bot commented Apr 8, 2021 •
edited

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/excalidraw/excalidraw/CmGnvyfPTiAjjARN2Te7r7FHGUX5
✅ Preview: https://excalidraw-git-fork-heitara-feature-image-element-excalidraw.vercel.app

@heitara heitara changed the title Feature/image element feat: Feature/image element Apr 8, 2021
@heitara
Copy link
Contributor Author

heitara commented Apr 8, 2021

#1 from the list above has been resolved.
#2 - If someone can demystify the idea with isDeleted:true then I might figure out where to add a handler that should update the image cache.

@heitara heitara mentioned this pull request Apr 9, 2021
Closed
@heitara
Copy link
Contributor Author

heitara commented Apr 9, 2021 •
edited

Adding the technical requirements from issue #19 to the PR for better visibility.

Technical

  • The image source data (the base64 or whatever format) shouldn't be stored in elements array, but live in a separate data structure (likely in appState.images).
  • The image source will be linked to an element (of {type: "image"} or something) that will live in elements — this element will be used for manipulation, and act as a normal element would.
  • In collab, the image source shouldn't be synced more than once per session. Only the element in elements array should be synced freely as we normally do with other elements.
  • gzipping the image will actually offer almost no benefits, so let's not do that. But we'll want to serialize to binary string instead of base64 (which is inefficient). But we may also consider lossy compression (thus encode as JPG).
  • There should be a limit on individual size, and likely a limit on total images per scene.
  • When persisting the image to server (e.g. for collab scenes), we're currently using Firebase Firestore, which has 1MB size limit on documents. So we'll need to persist the images to separate Firestore documents, or preferably to Firebase Storage which is more suited for resources such as images (it also has better pricing per GB).
  • [EXPORT/SAVE] we should cover the save to file and load from a file.
  • [TEST] Add tests.

UI

  • For the first iterration we don't want any UI changes and adding images should happen with drag and drop (and maybe in the context menu to support mobile)
  • Show additional information in stats
    - Total number of images
    - Total size of images
    - When selecting an image or two.. show their size.

@heitara
Copy link
Contributor Author

heitara commented Apr 9, 2021 •
edited

Commit acdc346 (acdc346) breaks the behavior of image import in Firefox. (The "browser-nativefs" library is working differently compared to "browser-fs-access", but the tests are failing.) This should be further investigated.

@heitara
Merge branch 'master' into feature/image-element
df3f431 
* master: (159 commits)
  fix: deselect elements on viewMode toggle (excalidraw#3741)
  fix: allow pointer events for disable zen mode button (excalidraw#3743)
  feat: expose fontfamily and refactor FONT_FAMILY (excalidraw#3710)
  feat: Show active file name when saving to current file (excalidraw#3733)
  feat: add hint around text editing (excalidraw#3708)
  chore(deps-dev): bump ts-loader in /src/packages/excalidraw (excalidraw#3716)
  chore(deps-dev): bump ts-loader in /src/packages/utils (excalidraw#3712)
  chore(deps-dev): bump typescript in /src/packages/excalidraw (excalidraw#3671)
  chore(deps-dev): bump @babel/plugin-transform-typescript (excalidraw#3713)
  chore(deps-dev): bump @babel/preset-env in /src/packages/utils (excalidraw#3714)
  chore(deps-dev): bump @babel/plugin-transform-async-to-generator (excalidraw#3715)
  chore(deps): bump ws from 7.4.3 to 7.4.6 in /src/packages/excalidraw (excalidraw#3665)
  chore(deps-dev): bump webpack in /src/packages/excalidraw (excalidraw#3670)
  chore(deps): bump ws from 7.4.3 to 7.4.6 in /src/packages/utils (excalidraw#3664)
  chore(deps-dev): bump autoprefixer in /src/packages/excalidraw (excalidraw#3672)
  chore(deps-dev): bump webpack in /src/packages/utils (excalidraw#3673)
  chore(deps-dev): bump @babel/preset-env in /src/packages/utils (excalidraw#3675)
  feat: change library icon to be more clear (excalidraw#3583)
  chore: Update translations from Crowdin (excalidraw#3659)
  fix: use excal id so every element has unique id  (excalidraw#3696)
  ...

# Conflicts:
#	package.json
#	src/components/App.tsx
#	src/data/restore.ts
#	src/element/collision.ts
#	src/element/types.ts
#	src/keys.ts
#	src/locales/en.json
#	src/renderer/renderElement.ts
@Tozuko
Copy link

Tozuko commented Jun 20, 2021

@heitara What state would you say this is in? I could potentially try to help finish it, but just wanted to know how far it's diverged from master and what your plans are.

@dwelle
Copy link
Member

dwelle commented Jun 20, 2021

We should be able to start working on this within a week. We need to wrap a couple of things first, and decide on how images should work with persistence. But it's definitely coming!

@heitara
Copy link
Contributor Author

heitara commented Jun 20, 2021

@Tozuko I've merged the latest changes from master and will push it here. I'm about to refactor some bits - more precisely where the image is stored.

@dwelle dwelle linked an issue Aug 17, 2021 that may be closed by this pull request
Import images #19
Closed
@walking-octopus
Copy link

walking-octopus commented Sep 6, 2021 •
edited

Sadly, collaboration with images is not working. When would it be ready? Also, the last two previews do not work on Firefox.

@dwelle
Copy link
Member

dwelle commented Sep 6, 2021

@walking-octopus collaboration is being worked on. Can you give more details about Firefox (your version, whether there are any visible errors, or errors written to the developer console)? It works for me on FF 91.

@heitara
Copy link
Contributor Author

heitara commented Sep 6, 2021

I can confirm that a local build works fine in latest FF as well.

@ethanneff
Copy link

It has been a week since the last update. Is there anything the community or I can do to help this PR along?

@ad1992
Copy link
Member

ad1992 commented Sep 13, 2021

It has been a week since the last update. Is there anything the community or I can do to help this PR along?

@ethanneff thanks for the patience :). We will be resuming this soon and since there were lot of changes / rewrite on this PR so we will be opening a fresh PR to discuss and collaborate better with detailed check list, once thats done you can also help if interested.

@dwelle dwelle mentioned this pull request Sep 27, 2021
Merged
47 tasks
@nhaouari nhaouari mentioned this pull request Oct 1, 2021
Closed
@dwelle
Copy link
Member

dwelle commented Oct 22, 2021

replaced and shipped by #4011 — the implem ended up pretty much completely rewritten, but nevertheless, thanks @heitara and and @kbariotis for the leg up! ❤️

@dwelle dwelle closed this Oct 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dwelle dwelle Awaiting requested review from dwelle

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Import images
6 participants
@heitara @Tozuko @dwelle @walking-octopus @ethanneff @ad1992