Skip to content
This repository has been archived by the owner on Jan 3, 2023. It is now read-only.

Make image version query a timestamp fixes #60 #63

Merged
merged 4 commits into from
Aug 9, 2015

Conversation

Galadirith
Copy link
Collaborator

The rerendering of the preview was also decoupled from the pathwatcher
listener. Now all pathwatcher events are captured and processed
including all those triggered by the same file modification as described
in atom/node-pathwatcher#50.

The rerendering of the preview was also decoupled from the pathwatcher
listener. Now all pathwatcher events are captured and processed
including all those triggered by the same file modification as described
in atom/node-pathwatcher#50.
@Galadirith Galadirith self-assigned this Aug 9, 2015
@Galadirith Galadirith added this to the 1.7.0 milestone Aug 9, 2015
@leipert
Copy link
Contributor

leipert commented Aug 9, 2015

If you want, I can have a look at the OSX specific problems :)

@Galadirith
Copy link
Collaborator Author

That would be great thanks @leipert

@leipert
Copy link
Contributor

leipert commented Aug 9, 2015

@Galadirith. The problem seems to be the following.

If you delete a picture on OSX the pathwatcher get's destroyed as well and therefore the version is not updated after restoring.

@Galadirith
Copy link
Collaborator Author

Ah ok, great thanks @leipert

@Galadirith
Copy link
Collaborator Author

@leipert I have a rather mucky fix that involves watching the directory of the image to observe if it is restored. However, while it was me who suggested this deletion, restoration cycle should be fixed, do you actually think it should be?

I suppose I have somewhat lost sight of the original issue #49; should our only responsibility be to updating a file for the duration of its first life?

A very simple workaround to making an image show up if it either never existed and then was created, or was deleted and then restored is to simply force a re-render of the preview (by either toggling or editing the source), and then your if not imgVersion[src] > 0 will catch both of those case. This is obviously much better than reloading Atom in any case.

If you think that is reasonable, let me know, I'll strip out the delete, restore cycle specs. Thanks @leipert.

@leipert
Copy link
Contributor

leipert commented Aug 9, 2015

@Galadirith As my latest commit seems to be failing on linux and windows as well and is rather slow, let's just do the following: Reverse my commit and strip out those specs we do not need.

We just have to make sure that the most recent image is shown if the preview is opened or the image is changed.

@leipert
Copy link
Contributor

leipert commented Aug 9, 2015

So maybe add a spec which handles the following:

  1. Open Preview with picture
  2. Remove/rename picture
  3. Close Preview
  4. Re-Create picture
  5. Open Preview
  6. Latest version of picture is shown

@leipert
Copy link
Contributor

leipert commented Aug 9, 2015

Maybe we also do not need between a ?v=timestamp and ?v=deleted and just use ?v=timestamp, as we actually do not care whether the change was a delete or something else.

@Galadirith
Copy link
Collaborator Author

Yeh I would agree :D, I'll make the changes.

@leipert
Copy link
Contributor

leipert commented Aug 9, 2015

Okay. Once it works again, you can maybe push the new version to the users

@Galadirith
Copy link
Collaborator Author

Great, will do :D

…directory instead of the file."

This reverts commit bd24496.
Restored images may be either images that we're previously renamed or
previously deleted. To force a restored image to preview a rerender
through either a preview toggle or source edit is required.

Note that on windows pathwatcher does still watch files even after their
their path no longer exists, updating the preview even for restored
images withour a forced rerender, but on OSX it does not.
@Galadirith Galadirith merged commit db6a522 into master Aug 9, 2015
Galadirith added a commit that referenced this pull request Aug 9, 2015
@leipert leipert deleted the uts-image-version branch August 9, 2015 21:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants