Use transferred file type for created file if necessary #748

tobous · 2019-11-08T01:40:28Z

[INTERNAL][CORE] Make variable for encoding final in FileActivity

[FIX][I] #683 Introduce file type field for FileActivity

Adds the field fileType to FileActivity. This field can be used to
transfer the file type associated with the file, which is necessary in
cases where there is no file type automatically associated with the
created file on the receiving side.

This field is currently only used/needed by Saros/I, but I did not see
another sensible way of fixing #683. Look at the discussion of PR #748
for more information.

Preparation for fixing #683.

[FIX][I] #683 Use transferred file type for created file if needed

Adjusts the logic creating new files as a reaction to a received
activity by using the transferred file type for the newly created file
if there is no default file type association.

This was done to avoid situations where we could not obtain a document
representation of a newly created file as it had no file type associated
with it, leading to a desync as no content could be read from or written
to the file.

Fixes #683.

Adds the field fileType to FileActivity. This field can be used to transfer the file type associated with the file, which is necessary in cases where there is no file type automatically associated with the created file on the receiving side. This field is currently only used/needed by Saros/I, but I did not see another sensible way of fixing #683. For more information on this, you can have a look at PR #748. Preparation for fixing #683.

Adjusts the logic creating new files as a reaction to a received activity by using the transferred file type for the newly created file if there is no default file type association. This was done to avoid situations where we could not obtain a document representation of a newly created file as it had no file type associated with it, leading to a desync as no content could be read from or written to the file. Fixes #683.

tobous · 2019-11-08T02:02:13Z

As already mentioned in the commit messages, the solution approach uses components in the core, even though the information is currently only needed for Saros/I. I pondered a lot on this but could not come up with a better solution, so I chose the only one I deemed as sensible/possible.

The detailed issue description can be found in #683, but as a TL;DR:
If a shared file without a known file ending is created, the IDE asks the creator which file type to associate with the file. This association is needed to create a document for the file, which in turn is needed for us to interact with the file during the session. This association does not happen for the other session participants on the receiving side, meaning we can't read from or write to the file, leading to a desync.

My other solution approaches were:

Asking the used to choose a file type
This does technically work, but requires the user to successfully exit the dialog, meaning we would have to ask in a loop or end the session on dialog "failure". Furthermore, this gets quite annoying as it takes focus from the user when another user creates such a file, meaning it disturbs the normal workflow. (This is exacerbated in cases where multiple such files are created, e.g. as part of the session negotiation, leading to a flood of blocking dialogs that all have to be processed.)
Queuing activities until the local user has chosen a file type
The user has to choose a file type the first time they open the file, meaning we could queue all activities until such a time. This should technically also work, but gets very messy due to the following complications:
- Not all activity consumers are implemented in the IDE parts. Some consumers (like the checksum stuff) is implemented in the core, meaning I don't have access to the from the Saros/I part. This means that this solution would also require at least some changes to the core to accommodate the queuing logic.
- The queuing logic would be quite complicated as it would have to hook into every activity consumer to intercept the activities if necessary. Furthermore, the activities would later on also have to be passed back into the correct consumers in the correct order.

If you have other ideas, feel free to make a suggestion. ☺️

core/src/saros/activities/FileActivity.java

srossbach · 2019-11-08T04:27:13Z

core/src/saros/activities/FileActivity.java

@@ -49,6 +51,8 @@
   * @param content content of the file denoted by the path (only valid for {@link Type#CREATED} and
   *     {@link Type#MOVED})
   * @param encoding the encoding the content is encoded with or <code>null</code>
+   * @param fileType the file type of the new file (only used for {@link Type#CREATED} and {@link


Why is this necessary for file moves ?

When I wrote the SharedResourcesManager (component handling incoming file activities), I was acting under the assumption that it should mainly operate on the Saros API.

As a result, received file moves are handled through the saros IResource API, meaning the move is realized through the creation of a new resource and the deletion of the old resource. If the old file initially had no file type association, the new file won't have one either, meaning we need to set it manually.

tobous · 2019-11-08T05:01:02Z

Also, there is still a corner case that this patch does not cover: when a file without a default file type association is created for another user as part of the recovery. As the recovery activities are created in the core, I did not see any way of also covering this without introducing another way of accessing the file type of resources (e.g. through the IFile interface), which seemed unnecessary to me.

But, this makes the rare case where such a recovery is necessary quite nasty as the session will instantly desync again until the other participant opens the file and chooses a file type (at which point the session might already be broken beyond repair; the behavior seems somewhat inconsistent in this case; but I also haven't tested it that much).

Still, this would be a corner case of a corner case, so I am currently leaning towards just ignoring it. But, if this is the way we decide to "deal" with this issue, I should note it in the commit messages and maybe also add a comment in the code.

stefaus · 2019-11-12T21:18:02Z

core/src/saros/activities/FileActivity.java

@@ -36,6 +36,8 @@

  @XStreamAsAttribute protected final String encoding;

+  @XStreamAsAttribute protected final String fileType;


Please document that this is only used in IntelliJ and maybe link to #683.

@prechelt suggested that this could be replaced with an isBinary() flag as this piece of information can be provided IDE-independently. This flag would then be used to open the file as a normal text file on the other side if it is not a binary file, allowing for a base document type to work with. If the user then want's a specific syntax highlighting, they could still change the file type association by hand.

The big improvement of the approach is that it would be supported across IDEs, meaning it would not cause any issues in cross-IDE sessions.

This sounds like a good compromise between providing the best user experience and working towards cross-IDE-compatibility to me. I will check whether this approach is doable and adjust the PR accordingly.

I can still add a note that it is currently only used in IntelliJ with the new logic, but I am not sure that it is still needed in that case as the provided information is IDE-independent.

Ok, this is not as easy as I first thought. Apparently Eclipse has a more relaxed approach to handling unknown file types. If it does not know the extension (or there is no extension), it just opens it as a plain text document. And from having a brief look at their IResource and IFile API, I couldn't find a method to determine the file type besides getFileExtension(), which simply returns the file extension if it exists.

So it seems like we have to guess whether a resource is binary or not (i.e. use other libraries to determine it). This also leads to other compatibility issues down the line where Eclipse simply opens an editor for a file where IntelliJ won't as it sees it as a binary file.

Eclipse do not has a relaxed approach. You register document providers, telling Eclipse that you can handle file extension XYZ. If there is no such document provider the default one is used which simply loads the content of the (binary) file. So the Eclipse File API is not relevant for the Editor stuff.

Please document that this is only used in IntelliJ and maybe link to #683.

I do not think that it is very helpful to add specific IDE documentation into the core. In general this is an optional hint, i.e the IntelliJ part should not assume that it contains any content when regarding cross IDE-support.

@srossbach Do you know whether there is an Eclipse API for determining whether an IFile is a binary file or not?

I do not think so. This is the by far the lowest implementation you can access https://help.eclipse.org/kepler/index.jsp?topic=%2Forg.eclipse.platform.doc.isv%2Freference%2Fapi%2Forg%2Feclipse%2Fcore%2Ffilesystem%2FEFS.html and it seems to only support Unix like attributes for files.

Only found this on Stackoverflow https://stackoverflow.com/questions/36018866/how-to-determine-if-a-file-is-a-text-file-in-eclipse

But the Eclipse documentation regarding the classes is not very helpful.

stefaus · 2019-11-12T21:19:37Z

core/src/saros/activities/FileActivity.java

+  /**
+   * Returns the file type for the file.
+   *
+   * @return the file type for the file


please doc possible null

stefaus · 2019-11-12T21:28:51Z

core/src/saros/activities/FileActivity.java

@@ -57,7 +61,8 @@ public FileActivity(
      SPath newPath,
      SPath oldPath,
      byte[] content,
-      String encoding) {
+      String encoding,
+      String fileType) {


nit: Nothing new but we probably should replace this CTOR with static factory methods. Depending on the FileActivity.Type different parameters are valid.

tobous · 2020-02-25T12:54:44Z

Approach needs to be re-worked and generalized for cross-IDE support.

[INTERNAL][CORE] Make variable for encoding final in FileActivity

be95533

tobous added Type: Bug Issue that describes an unintended behavior of Saros Aspect: Consistency Issue with Saros' consistency handling Area: IntelliJ Issue affecting Saros for IntelliJ (Saros/I) Area: Core Issue affecting the Saros Core labels Nov 8, 2019

tobous added this to the Saros/I Release 0.3.0 milestone Nov 8, 2019

tobous self-assigned this Nov 8, 2019

tobous added 2 commits November 8, 2019 02:41

tobous force-pushed the pr/intellij/set_file_type branch from f3f75e0 to 59ef31a Compare November 8, 2019 01:42

tobous requested review from m273d15, srossbach and stefaus November 8, 2019 02:02

srossbach reviewed Nov 8, 2019

View reviewed changes

stefaus reviewed Nov 12, 2019

View reviewed changes

tobous closed this Feb 25, 2020

tobous removed this from the Saros/I Release 0.3.0 milestone Mar 31, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use transferred file type for created file if necessary #748

Use transferred file type for created file if necessary #748

tobous commented Nov 8, 2019

tobous commented Nov 8, 2019 •

edited

Loading

srossbach Nov 8, 2019

tobous Nov 8, 2019

tobous commented Nov 8, 2019 •

edited

Loading

stefaus Nov 12, 2019

tobous Nov 13, 2019

tobous Nov 13, 2019

srossbach Nov 13, 2019

srossbach Nov 13, 2019

tobous Nov 13, 2019

srossbach Nov 13, 2019

srossbach Nov 13, 2019

stefaus Nov 12, 2019

stefaus Nov 12, 2019

tobous commented Feb 25, 2020

		@@ -36,6 +36,8 @@

		@XStreamAsAttribute protected final String encoding;

		@XStreamAsAttribute protected final String fileType;

Use transferred file type for created file if necessary #748

Use transferred file type for created file if necessary #748

Conversation

tobous commented Nov 8, 2019

[INTERNAL][CORE] Make variable for encoding final in FileActivity

[FIX][I] #683 Introduce file type field for FileActivity

[FIX][I] #683 Use transferred file type for created file if needed

tobous commented Nov 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobous commented Nov 8, 2019 • 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

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

tobous commented Feb 25, 2020

tobous commented Nov 8, 2019 •

edited

Loading

tobous commented Nov 8, 2019 •

edited

Loading