add onPrev, onNext, and onClose callbacks #497

azul · 2020-05-11T10:00:15Z

Apps using the viewer might want to react to changes in the viewer.

In particular make use of the callbacks
to updating the url in the files app when navigating between files.
This way the url can be copied and used to navigate to one particular image in a collection.

It also can be used to add browser history entries.
For this first call the callback and then update the title.
This way the callback can create a new history entry
and the title of that entry will then be updated.

onPrev and onNext callbacks receive the old and the current fileInfo
as arguments.
onClose callback does not receive any arguments as of now.

Pull Request consists of two commits:

introduce the callbacks
make use of them when defining the actionHandler.

This pull request is a result of the discussion in #488

src/views/Viewer.vue

azul · 2020-05-12T12:44:57Z

src/views/Viewer.vue

@@ -222,7 +223,7 @@ export default {
 			if (path.trim() !== '') {
 				console.info('Opening viewer for file ', path)
 				this.openFile(path)
-			} else {
+			} else if (this.initiated) {


oh... one more thing. I had to add this condition because the close function was getting twice. I suspect it happened like this:

clicking the ❌ triggers the close function

the close function indirectly sets file to '' which triggers this observer function.

Since close also sets this.initiated to false this condition works to prevent calling close again. However it seems like a workaround for a more fundamental problem.

Should i try and solve the underlying problem as part of this pr?
What would be the best way to approach it?

Yes, if you can narrow why is it called twice, that would be better I think :)
Thanks!

I figured out what happens:

clicking the ❌ triggers the close function

close function calls OCA.Viewer.close() and triggers the callback

OCA.Viewer.close() sets file to ''

the file watcher triggers the close function again

close function calls OCA.Viewer.close() and triggers the callback again

OCA.Viewer.close() sets file to ''

this time it has no effect because it already was empty.

In order to fix this i separate cleanup from the close function

close() delegates to OCA.Viewer.close().
It is just a shortcut to be used in the template.

cleanup() does the actual work.
OCA.Viewer.close() will set file to ''.
file is being watched and the watch function triggers the cleanup.

azul · 2020-06-24T20:20:31Z

@skjnldsv If you could take another look... that would be great 😃

(added another commit fixing the double call of onClose callback.)

skjnldsv · 2020-07-07T12:38:41Z

@skjnldsv If you could take another look... that would be great

Sorry, I'm overwhelmed since a few months! :(
I definitely did not forget about this pr :)
Thanks for the heads up!!

azul · 2020-07-07T12:56:51Z

Sorry, I'm overwhelmed since a few months! :(

That's what i was assuming. I know how it feels. Take your time. 🍵 🥰

src/views/Viewer.vue

skjnldsv · 2020-07-26T11:10:27Z

src/services/Viewer.js

 	 * @param {string} path the path to open
 	 * @param {Object[]} [list] the list of files as objects (fileinfo) format


Could you also update the jsdoc, it is outdated and doesn't reflect the Object params requirements (I forgot to update it before, but since we're at it 🙈)

src/services/Viewer.js

skjnldsv · 2020-07-26T11:11:48Z

Please also rebase! :)
A few comments and reviews, let's try to get this in soon so you don't feel frustrated by the wait! 😉
Thanks again for your patience!! 🤗

skjnldsv · 2020-07-27T08:41:17Z

Let's rebase, squash and get this in? :)

azul · 2020-07-27T08:42:50Z

@skjnldsv I'm in a call right now. This is the next thing I'll work on.

azul · 2020-07-27T10:24:16Z

@skjnldsv I rebased and updated the jsdoc.
I'm not so familiar with jsdoc syntax. So please take a look if it's correct 👓
Shall I squash everything into a single commit?

skjnldsv · 2020-08-01T07:58:49Z

Shall I squash everything into a single commit?

Yes please :)

skjnldsv · 2020-08-01T07:59:05Z

Btw @azul , this also fixes #497, right?

azul · 2020-08-02T09:03:51Z

✔️ squashed
✔️ rebased again

Btw @azul , this also fixes #497, right?

This is #497 - I tried to find the issue you meant to point at but did not see it.

@skjnldsv - ready to merge?

skjnldsv · 2020-08-02T10:55:55Z

This is #497 - I tried to find the issue you meant to point at but did not see it.

sorry, #486 😁

skjnldsv · 2020-08-02T10:57:43Z

/compile amend /

Apps using the viewer might want to react to changes in the viewer. In particular this will update the url in the files app when navigating between files. This way the url can be copied and used to navigate to one particular image in a collection. It also can be used to add browser history entries. For this to work it is important to first call the callback and then update the title. This way the callback can create a new history entry and the title of that entry will then be updated. onPrev and onNext callbacks receive the old and the current fileInfo as arguments. onClose callback does not receive any arguments as of now. Update the url by pushing a new entry to browser history. Update before the initial load, when navigating to include the fileid of the newly displayed file. Push the original url again once the viewer is closed. `close()` delegates to `OCA.Viewer.close()`. It is just a shortcut to be used in the template. `cleanup()` does the actual work. `OCA.Viewer.close()` will set `file` to `''`. `file` is being watched and the watch function triggers the cleanup. This way each step is only getting called once. Here's what was happening before: * clicking the ❌ triggers the close function * close function calls `OCA.Viewer.close()` and triggers the callback * `OCA.Viewer.close()` sets `file` to `''` * the `file` watcher triggers the close function again * close function calls `OCA.Viewer.close()` and triggers the callback again * `OCA.Viewer.close()` sets `file` to `''` * this time it has no effect because it already was empty. doc: update jsdoc for Viewer.open Signed-off-by: Azul <azul@riseup.net> Signed-off-by: npmbuildbot[bot] <npmbuildbot[bot]@users.noreply.github.com>

azul · 2020-08-02T18:04:21Z

@skjnldsv It's a first step towards solving #486. Now we add browser history entries for the files we navigate to. But the files app still needs a handler for dealing with clicks to the browser back button that make the viewer display the file. I'll work on that as one of the next things.

skjnldsv · 2021-03-30T09:09:08Z

src/views/Viewer.vue

+		},
+
+		onNext(info, oldFileInfo) {
+			this.Viewer.onNext(oldFileInfo)


Hey @azul, I just rezlized we did a mistake, the onPrev and onNext have different parametters.
It should most likely be consistent:

this.Viewer.onNext(info, oldFileInfo)

azul force-pushed the callbacks branch 2 times, most recently from 44cd8d5 to 4acdf8f Compare May 11, 2020 12:18

azul requested a review from skjnldsv May 11, 2020 12:21

skjnldsv reviewed May 11, 2020

View reviewed changes

src/views/Viewer.vue Outdated Show resolved Hide resolved

skjnldsv reviewed May 11, 2020

View reviewed changes

src/views/Viewer.vue Outdated Show resolved Hide resolved

azul force-pushed the callbacks branch from 4acdf8f to 6f588c9 Compare May 12, 2020 11:12

azul requested a review from skjnldsv May 12, 2020 11:15

azul commented May 12, 2020

View reviewed changes

azul mentioned this pull request Jun 19, 2020

Link to local files nextcloud/text#879

Merged

6 tasks

azul force-pushed the callbacks branch 3 times, most recently from 2464d5c to dc72592 Compare June 22, 2020 08:13

skjnldsv added 2. developing Work in progress enhancement New feature or request labels Jul 7, 2020

skjnldsv reviewed Jul 26, 2020

View reviewed changes

src/views/Viewer.vue Outdated Show resolved Hide resolved

skjnldsv reviewed Jul 26, 2020

View reviewed changes

src/services/Viewer.js Outdated Show resolved Hide resolved

azul force-pushed the callbacks branch 2 times, most recently from c66416e to fd03cec Compare July 27, 2020 10:22

azul requested a review from skjnldsv July 27, 2020 10:24

azul force-pushed the callbacks branch from fd03cec to 812d77f Compare August 2, 2020 08:38

skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 2. developing Work in progress labels Aug 2, 2020

npmbuildbot-nextcloud bot force-pushed the callbacks branch from 812d77f to bcf6f74 Compare August 2, 2020 11:00

skjnldsv approved these changes Aug 2, 2020

View reviewed changes

skjnldsv merged commit d2ee1fa into nextcloud:master Aug 2, 2020

skjnldsv reviewed Mar 30, 2021

View reviewed changes

joshtrichards mentioned this pull request Jul 2, 2023

Web Browser Back Button in Image Preview unnoticeably acts on File Browser. #486

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add onPrev, onNext, and onClose callbacks #497

add onPrev, onNext, and onClose callbacks #497

azul commented May 11, 2020 •

edited

Loading

azul May 12, 2020 •

edited

Loading

skjnldsv Jun 16, 2020

azul Jun 22, 2020

azul Jun 22, 2020

azul commented Jun 24, 2020 •

edited

Loading

skjnldsv commented Jul 7, 2020

azul commented Jul 7, 2020

skjnldsv Jul 26, 2020

skjnldsv commented Jul 26, 2020

skjnldsv commented Jul 27, 2020

azul commented Jul 27, 2020

azul commented Jul 27, 2020

skjnldsv commented Aug 1, 2020

skjnldsv commented Aug 1, 2020

azul commented Aug 2, 2020

skjnldsv commented Aug 2, 2020

skjnldsv commented Aug 2, 2020

azul commented Aug 2, 2020

skjnldsv Mar 30, 2021 •

edited

Loading

		* @param {string} path the path to open
		* @param {Object[]} [list] the list of files as objects (fileinfo) format

add onPrev, onNext, and onClose callbacks #497

add onPrev, onNext, and onClose callbacks #497

Conversation

azul commented May 11, 2020 • edited Loading

azul May 12, 2020 • edited Loading

Choose a reason for hiding this comment

skjnldsv Jun 16, 2020

Choose a reason for hiding this comment

azul Jun 22, 2020

Choose a reason for hiding this comment

azul Jun 22, 2020

Choose a reason for hiding this comment

azul commented Jun 24, 2020 • edited Loading

skjnldsv commented Jul 7, 2020

azul commented Jul 7, 2020

skjnldsv Jul 26, 2020

Choose a reason for hiding this comment

skjnldsv commented Jul 26, 2020

skjnldsv commented Jul 27, 2020

azul commented Jul 27, 2020

azul commented Jul 27, 2020

skjnldsv commented Aug 1, 2020

skjnldsv commented Aug 1, 2020

azul commented Aug 2, 2020

skjnldsv commented Aug 2, 2020

skjnldsv commented Aug 2, 2020

azul commented Aug 2, 2020

skjnldsv Mar 30, 2021 • edited Loading

Choose a reason for hiding this comment

azul commented May 11, 2020 •

edited

Loading

azul May 12, 2020 •

edited

Loading

azul commented Jun 24, 2020 •

edited

Loading

skjnldsv Mar 30, 2021 •

edited

Loading