update history when opening and closing viewer

[azul]

• Add the openfile param to the url when opening a file
• update the document title from the Mime mixin
• push the new url to the browser history
• Also update the title and push the url when moving inside a collection
• push original url to browser history state when closing the viewer

fixes nextcloud/server#12470

Alternatives

We could pop state back to where we started when closing the viewer.
That way history would not contain urls for visited files.
In particular when navigating through collections
I think having a full history may be useful

Warning

Adds the following warning:

/home/next/code/nextcloud/server/apps/viewer/src/mixins/Mime.js
  111:20  warning  The property or function OC.encodePath was deprecated in Nextcloud 18.0.0

I tried to work around it
by using encodePath from https://www.npmjs.com/package/@nextcloud/paths
But no external imports are allowed in mixins.

Next Steps

• Handle popstate events so we can actually go back in history

[skjnldsv]

Hi @azul !
Thanks for your pull request! :)

I'm not sure this is the road to take for this to be honest.
I never seen any slideshow remembering the history and allow navigating through it. I would just close the slideshow on delete keypress.

@jancborchardt ?

[azul]

@skjnldsv my main focus was on getting an actionable url when opening the viewer.
I'm happy to switch to replaceState for changing inside the slideshow and popState for closing.

In terms of what others do:

• flickr.com keeps history
• wikipedia.org does not (in the images for the article slideshow).

[skjnldsv]

• Messenger does not
• Whatsapp does not
• Google drive does not
• Google photos does not

I would only therefore add a click listener and close the viewer on it.
No need for history manipulation :)

[jancborchardt]

I never seen any slideshow remembering the history and allow navigating through it.

• Facebook allows it, so that alone I’d say is already an argument for us doing it
• Instagram as well, try going to https://www.instagram.com/unwomen/, click one of the photos and navigate left/right with the outer navigation arrows (that said it always has the same title, so we should be more useful here, e.g. with "filename - Photos - Nextcloud")

[skjnldsv]

I didn't took those example because both of them have different behavior. The urls changes because you can access the ressources directly. But we don't, so the navigation will make no sense as reloading will lead to a different view.

Nextcloud doesn't offer to access the viewer directly. If there was, sure, this feature would make sense :)

[azul]

@skjnldsv There is a way to access files opened in the viewer directly now. The urls i am adding also work on reload / copied to another browser window.
nextcloud/server#20185 added this abiity. The main reason for me to propose this change is to make the url more visible / easy to copy as proposed by @juliushaertl in the discussion for nextcloud/server#12470.

[skjnldsv]

Aaah, I was not aware this got in! Nice!
In any way, the viewer is used in other areas than files, meaning this doesn't make sense to have this as a standard accross the viewer.
If I open a photo in Talk, and refresh, (if I understand this correctly), I will get redirected to files? That will then load the image correct?

[azul]

One thing that is still not working with this commit though is navigating back in history to a given image. This will require an update to the servers history.onpopstate handler.
I'd be happy to work on that next. I would also try to fix #486 then.

[azul]

@skjnldsv :

If I open a photo in Talk, and refresh, (if I understand this correctly), I will get redirected to files? That will then load the image correct?

I have not tried this. I will give it a try. I think we could make it work in talk as well. But that would require changes to talk then.

[azul]

So my current thinking is this:

• when opening a file in the viewer add the openfile=132 param to the current url
• when navigating to another file (f.e. in a slideshow) update the openfile param
• when closing the viewer remove the openfile param from the current url

Moving in history is handled by onpopstate handlers provided by both the underlying app and the viewer app:

• the underlying app itself has a handler that loading the app content itself
• the viewer app has a handler that:
  • closes an open viewer if the new url does not have an openfile param ( fixes Web Browser Back Button in Image Preview unnoticeably acts on File Browser. #486 )
  • opens the viewer with the given file if the url does have an openfile param

Optionally OCA.Viewer.open could have an updateUrl={true,false} param that turns the addition of the openfile param on or off. If the Viewer was opened with updateUrl=false the url should remain unchanged and no new history entries should be added when changing the file or closing the viewer.

Apps that want to react to openfile params in their urls would have to check for the openfile param like the files app does. This could be eased by providing a helper function such as Viewer.openFileFromParam().

The simplified openfile param logic described above would require a fix for /nextcloud/server#20818 .

[skjnldsv]

Thanks for the insight @azul :)
I still think any kind of specific methods applied to Viewer only but that have a reach to other apps is a bad way of implementing this. This would just polute the Viewer with unwanted code that have no real effect.
Also would make it hard to change as other apps will all needs update if this moves.

---

Here's my proposal.
On the Viewer open, allow two more properties: onPrev, onNext. Call them every time we change image if those are defined and functions.
As arguments, pass the new FileInfo and the previous FileInfo.

Then on files, you can register the full behavior you wanted with the history.

• Viewer stays clean
• We don't introduce properties/methods that don't make sense to Viewer
• We now let the apps do whetever they want with it. Including history management, but also any kind of future stuff.
• Future proof

[azul]

@skjnldsv Your proposal sounds good and clean to me.
In addition i would like to remove the title handling from the viewer. Instead the apps could also update the displayed title in these hooks.

It's a similar situation where it's difficult for the viewer app to guess the correct title to display.

Title handling and history operations are closely related as the current history entry will be shown with the title it had when the next entry is added. So what one usually wants to do is first push the new history entry and then update the title.

Oh... one more thing comes to my mind... We might also need a close callback that gets called when the viewer is closed again - or is there already a way to detect this?

[azul]

Another thought.... do we need to differenciate between onPrev and onNext ?
What about the initial loading? Obviously one could call Viewer.open() and then update the title and the url. But for Viewer.open() one only needs the path - that does not imply one knows the full file info. So this might still be useful. It would also be good to update the title and the url after the viewer actually loaded and not after Viewer.open() was triggered to avoid inconsistencies.

So how about a onLoaded or onUpdated property that gets called every time the viewer content changes?
Another reason for this is that in the future there might be additional ways of navigating between files in the viewer. Say jump to the first image in a collection, or follow a link in a markdown document to a file in the same dir. So onPrev and onNext seem pretty specific in that regard.

[skjnldsv]

I wouldn't mix everything.
Please only manage the history there.
We currently don't have a dedicated standard n how we manage titles, I prefer leaving this to Viewer for now.

[azul]

@skjnldsv I just noticed that i can use Vue.js to watch Viewer.state.file. It will change from empty string to the path when opening the viewer and back to empty string when closing.
If state.file could be updated when navigating between files this would be an easy way to observe this as well.

Is there a reason for keeping the file that was opened first in Viewer.state?
Would it be okay to expose the fileInfo of the currently open file in Viewer.state?

[skjnldsv]

If state.file could be updated when navigating between files this would be an easy way to observe this as well.

It should be updated indeed 🤔
But You should not use Viewer.state, this is not here for that.
This is here for Vue reactivity binding only. Looking at this, I now think I made a typo and should have used OCA.Viewer instead of this.Viewer 😕

[azul]

But You should not use Viewer.state, this is not here for that.
This is here for Vue reactivity binding only.

What i was doing is a reactivity binding from the files app just to see if it works:

export default {
	name: 'History',
	components: {},
	data() {
		return {
		    Viewer: OCA.Viewer.state,
		}
	},
	computed: {
		file() {
			return this.Viewer.file
		},
	},
	watch: {
		file: function(val, oldVal) {
			alert(val + ' <- ' + oldVal)
		},
	},
}

But I don't know if this kind of reactivity binding between apps is a good idea or not. On the one hand i think the state is somewhat internal to the viewer. On the other hand it would allow for watching such things really easily and the 'normal' vue.js way.

[skjnldsv]

I don't understand what you're doing, this is against what I told in #488 (comment) ?

[azul]

I don't understand what you're doing, this is against what I told in #488 (comment) ?

Okay... sorry. I see how this can be confusing. I'll try to clarify.

I got back about what you told me above in #488 (comment) and #488 (comment) . Main point being that in order to get the History handling working we'd also need a way to trigger code when the initial file was opened and when the viewer was closed. (As these also should result in updates to the history). OnPrev and OnNext don't work here (or i don't understand how they would).

My suggestion would be something like OnDisplayed(fileInfo, prevFileInfo) and OnClosed(prevFileInfo) instead.

I continued looking at the code for further understanding and noticed that the exposed state of OCA.Viewer allows me to reactively bind to it and watch the changes i am looking for. I thought this might even be a cleaner - or at least more vue.js like approach that would require very little changes to the Viewer codebase.

My last comment was trying to explain the approach i took there to clarify if this is an expected use of state or not.

I'm happy to use and implement either approach. Just trying to make sure I don't add a callback based API when using the exposed state would be just fine.

[skjnldsv]

I'm happy to use and implement either approach. Just trying to make sure I don't add a callback based API when using the exposed state would be just fine.

That's what I was afraid.
The state is not exposed and is private for this specific reason :)
I don't want developers to do anything with it. We either provide a steady api or none at all.

---

So you need to add the proper onNext and onPrev callbacks, that is all we need :)

viewer/src/services/Viewer.js

Line 34 in b313993

this.#state.loadMore = () => ([])

No need to watch anything, you trigger them on the the already existing previous and next functions

viewer/src/views/Viewer.vue

Lines 609 to 628 in 0158091

	if (this.currentIndex < 0) {
	this.currentIndex = this.fileList.length - 1
	}
	this.openFileFromList(this.fileList[this.currentIndex])
	},
	/**
	* Open next available file
	*/
	next() {
	this.currentIndex++
	if (this.currentIndex > this.fileList.length - 1) {
	this.currentIndex = 0
	}
	this.openFileFromList(this.fileList[this.currentIndex])
	},
	/**

And regarding having to trigger on open, well, since the app trigerred the Viewer open, it will know when it's triggerred 😉

[skjnldsv]

I thought this might even be a cleaner - or at least more vue.js like approach that would require very little changes to the Viewer codebase.

I understand a bit more.
But actually, the api is not Vue :)
Viewer is written in Vue but we want the api to be fully accessible from any javascript code. So anyone can trigerrer it. That's why the onNext and onPrev callbacks makes more sense here. If someone want to write a pure javascript app, so be it. They won't have to implement any kind of component/mixin/watcher or compiled code for this. We should keep the low level code complexity low ;)

[azul]

Ok, I'll add onPrev, onNext and onClosed handlers as arguments to the open function in a separate pull request.