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

Compare 2 versions of a file #39140

Closed
AndyScherzinger opened this issue Jul 4, 2023 · 18 comments · Fixed by #39171
Closed

Compare 2 versions of a file #39140

AndyScherzinger opened this issue Jul 4, 2023 · 18 comments · Fixed by #39171
Assignees
Labels
2. developing Work in progress design Design, UI, UX, etc. enhancement

Comments

@AndyScherzinger
Copy link
Member

AndyScherzinger commented Jul 4, 2023

Provide an option to compare 2 versions of a file by marking them and opening both side-by-side in some kind of a "dual-pane", so the user can scroll both to visually compare them.

@AndyScherzinger AndyScherzinger added this to the Nextcloud 27.1.0 milestone Jul 4, 2023
@AndyScherzinger AndyScherzinger added enhancement 0. Needs triage Pending check for reproducibility or if it fits our roadmap labels Jul 4, 2023
@artonge
Copy link
Contributor

artonge commented Jul 5, 2023

What file formats should be covered by that feature?
Should the files be editable on that view? Might be problematic for files like spreadsheets.

One option could be to generate a picture of both files, compare those pictures to create a diff, and overlay the diff on both files. Might not be easy, but would be generic.

The non-generic way would be to implement a diff algo for each file format. Maybe there is some open source one available already.

@AndyScherzinger
Copy link
Member Author

The idea is to just display 2 versions next to each other for the ones where we have a viewer available.
Like with viewing the version see #39139 any version should be read-only except for the current version

@AndyScherzinger AndyScherzinger added the design Design, UI, UX, etc. label Jul 5, 2023
@jancborchardt
Copy link
Member

@nextcloud/designers it would be good to have a mockup for this, how to select the 2 versions and compare them. :)

@marcoambrosini
Copy link
Member

I'm taking this on :)

The requirements are not very clear from the issue so can we formulate a problem statement for this feature? What is the goal? Is the comparison of the two files oriented at picking one of the two?

Is it only for text files or also for other files like spreadsheets, pictures, videos and so on?

@jancborchardt
Copy link
Member

@marcoambrosini it would be a very simple side-by side comparison of 2 documents.

To keep it simple:

  • It is only possible to pick one version to compare to the current one. This would be done by an action menu entry "Compare version"
  • It is a side-by-side view, no diff or anything like that. The current version is on the right.
  • Changes can be made only to the current version, the other document is read-only.
  • We will just not make this possible on mobile, the option to "Compare version" will just open the version, as if you clicked on the name.
  • The individual views side-by-side could have individual x-close buttons, which would then lead to the other side taking back the whole width.

@artonge do you need mockups beyond this?

@AndyScherzinger
Copy link
Member Author

do you need mockups beyond this?

For the time being @juliushaertl's team will take over lending the files team a hand in getting this implemented quickly

@github-project-automation github-project-automation bot moved this to 🧭 Planning evaluation (don't pick) in 📝 Office team Jul 27, 2023
@AndyScherzinger AndyScherzinger moved this from 🧭 Planning evaluation (don't pick) to 📄 To do (~10 entries) in 📝 Office team Jul 27, 2023
@juliusknorr
Copy link
Member

juliusknorr commented Jul 31, 2023

No further input needed for now from the design side I think.

@skjnldsv This would require us to have some sort of API to trigger a side-by-side rendering in viewer. My suggestion also considering the short time available would be to simply add an optional parameter to pass a second file info object:

OCA.Viewer.open({
	fileInfo: currentFileInfo,
	fileInfoCompare: versionFileInfo,
})

This would align with the work already done in https://github.com/nextcloud/server/pull/39171/files#diff-8c16136a0fe1baf1357890af01439aebe1da2380f96f80afb1eab039157311b8R191-R198

It would then just use the existing viewer__content element and render an additional component for the fileInfoCompare passed object as a split view https://github.com/nextcloud/viewer/blob/ac5413b8cf4b60b8303182f2ea5226c1217dcc84/src/views/Viewer.vue#L114

Any concerns or objections on that?

@skjnldsv
Copy link
Member

skjnldsv commented Aug 1, 2023

I'm actually not sure about this.
I dont think this is a Viewer's job, no?

Any other app would benefit from this feature otherwise?
Aside from Text?

If this is about making it possible to view ANY file in the viewer as side-to-side, sure, then we need it in Viewer. WHile than can be really messy and the layout needs to be updated.
This creates lots of unresolved questions like:

  • Mobile view
  • Slideshow is disabled?
  • Videos makes no sense? (Or we sync the time position, but then it's a huge layer of complexity)
  • ... etc

We need clearer definitions of expectations please. As it seems supporting any mime seems like a bad idea, please provide which mime needs to be supported and we can tackle the technical dilemma :)

EDIT:
I see Jan's comment about it would be a very simple side-by side comparison of 2 documents. But I want to make sure we're on the same page. If this is about two texts only, then this is not a job for the Viewer imho.

TLDR

Does this viewer feature needs to be a documented standard:

  • yes ➡️ do it in Viewer
  • no ➡️ stay in your app

@juliusknorr
Copy link
Member

juliusknorr commented Aug 1, 2023

Any other app would benefit from this feature otherwise?
Aside from Text?
If this is about making it possible to view ANY file in the viewer as side-to-side, sure, then we need it in Viewer. WHile than can be really messy and the layout needs to be updated.

Yes, Collabora, ONLYOFFICE, files_pdfviewer would be obvious others, for image files I'd also consider it useful. But I agree that we probably want to exclude audio/video files for now.

This creates lots of unresolved questions like:

  • Mobile view
  • Slideshow is disabled?

Handling mobile view has been discussed above as that we just not show the split view then but switch to the single version view. Slideshow should probably just hide the split view and move to the next file. As a user you would need to pick a new version from the sidebar then anyways to compare to.

Does this viewer feature needs to be a documented standard:

Moving this to text or other apps seems likes adding a lot of complexity there for what is now a "simple" component to just render a file and would also mean to duplicate a lot of effort bringing this to more apps.

@skjnldsv
Copy link
Member

skjnldsv commented Aug 1, 2023

How would you load two times the UI of collabora/onlyoffice next to each others?
The layer of complexity would still be there, no?

Or do you have a preview component available that do simple rendering without any edit option?

@juliusknorr
Copy link
Member

Yes, there is a simplified preview mode that can be loaded separately. This needs some sort of support in the app to load from the passed source url (like https://github.com/nextcloud/files_pdfviewer/pull/646/files#diff-84f92d7129f879a2f201fbc32ac9281b7a84dc921ea4a1ba2754788046234b3a) instead of the file id which is otherwise used to initiate the editing session, but those parts are already there in the viewer from nextcloud/viewer#1338

@skjnldsv
Copy link
Member

skjnldsv commented Aug 1, 2023

Alright, considering the short deadline, OCA.Viewer.compare(currentFileInfo, versionFileInfo) would be the closest to the Node/File API usage that should come one day, so I recommend doing this

As a quick tip, I suggest using Permission.READ to have the views render the simplified preview mode instead of any custom prop ?

@AndyScherzinger
Copy link
Member Author

AndyScherzinger commented Aug 1, 2023

Anything but the current version shall always be read-only according to Frank and Jan (and I agree). If the current version could be editable (given the user has the permissions) would be a plus. So MVP can be read-only for everything.

What do you think @jancborchardt ?

@juliusknorr
Copy link
Member

juliusknorr commented Aug 2, 2023

Actually using the current file state editable is more straight forward from the implementation side as we can just reuse the existing viewer logic for that.

The read only mode hint would be for the version of the file then that is opened on the left of the comparison

@jancborchardt jancborchardt moved this from 📐 Design phase to 🏗️ At engineering in 🖍 Design team Aug 2, 2023
@AndyScherzinger AndyScherzinger moved this from 📄 To do (~10 entries) to 🏗️ In progress in 📝 Office team Aug 2, 2023
@juliusknorr
Copy link
Member

Some screenshots for the current state for design feedback @nextcloud/designers:

Clicking the version just opens a single preview

Screenshot 2023-08-03 at 15 29 51

Comparing menu

  • Comparing is available for any but the current version
    Screenshot 2023-08-03 at 15 27 46

  • Clicking it will compare the picked with the current version
    Screenshot 2023-08-03 at 15 27 50

@jancborchardt
Copy link
Member

Super nice @juliushaertl! I would say that's awesome for the MVP!

@AndyScherzinger AndyScherzinger moved this to 📄 To do (~10 entries) in 📁 Files team Aug 3, 2023
@AndyScherzinger AndyScherzinger moved this from 📄 To do (~10 entries) to 🏗️ In progress in 📁 Files team Aug 3, 2023
@AndyScherzinger AndyScherzinger added 2. developing Work in progress and removed 0. Needs triage Pending check for reproducibility or if it fits our roadmap labels Aug 3, 2023
@marcoambrosini
Copy link
Member

Very nice Julius!
The only thing I would add in the comparison view is info about which versions we're seeing in the title bar instead of the file name in the middle.

@github-project-automation github-project-automation bot moved this from 🏗️ In progress to ☑️ Done in 📝 Office team Aug 8, 2023
@github-project-automation github-project-automation bot moved this from 🏗️ In progress to ☑️ Done in 📁 Files team Aug 8, 2023
@github-project-automation github-project-automation bot moved this from 🏗️ At engineering to 🎉 Done in 🖍 Design team Aug 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing Work in progress design Design, UI, UX, etc. enhancement
Projects
Archived in project
Archived in project
Archived in project
Development

Successfully merging a pull request may close this issue.

6 participants