Allow registering aliases without components

[skjnldsv]

Signed-off-by: John Molakvoæ (skjnldsv) [email protected]

[ariselseng]

@skjnldsv
After putting in if (handler.component) { before line 385 to fix : TypeError: Cannot read property 'name' of undefined

I get The following handler is already registered when I do:

document.addEventListener('DOMContentLoaded', function() {
	OCA.Viewer.registerHandler({
		id: 'camerarawpreviews',
		mimes: ['image/x-dcraw'],
		mimesAliases: {
			'image/x-dcraw': 'image/jpeg'
		}
	})
})

[skjnldsv]

Don't register a mime, only the alias :)
@cowai

[ariselseng]

@skjnldsv
Thanks, so it works now, but still get the error The following handler is already registered
I needed to have an empty mimes array. group was also needed.

	OCA.Viewer.registerHandler({
		id: 'camerarawpreviews',
		group: 'media',
		mimes: [],
		mimesAliases: {
			'image/x-dcraw': 'image/jpeg'
		}
	})
})

[skjnldsv]

I pushed a fix, you should be able to register without a group (not needed for you since you want the media group I assume) and without a mimes array :)

[ariselseng]

@skjnldsv Thanks. It works fine now with only id and mimeAliases.

I still get The following handler is already registered

[skjnldsv]

With that? I don't have the issue. Did you build the viewer app ?
Make sure your app doesn't already register the same handler if you're doing the tests in the console :)

OCA.Viewer.registerHandler({
		id: 'camerarawpreviews',
		mimesAliases: {
			'image/x-dcraw': 'image/jpeg'
		}
	})

[ariselseng]

@skjnldsv Only doing it in the app. My code is only ran once. This branch is built.

If I try directly in the console there is no issue, but when doing it with DOMContentLoaded there is.

document.addEventListener("DOMContentLoaded", function() {
	if (OCA.Viewer) {
		console.log("TEST")
		OCA.Viewer.registerHandler({
		id: 'camerarawpreviews',
		mimesAliases: {
			'image/x-dcraw': 'image/jpeg'
		}
	})
	}
});

I only see "TEST" once in the console.

[skjnldsv]

Works here 🤷‍♀️

[ariselseng]

@skjnldsv It's a race condition between watch.handlers and beforeMount.

[ariselseng]

@skjnldsv
It's definitely a race condition here.
Putting my code in a setTimeout forces it to let Viewer load:

document.addEventListener("DOMContentLoaded", function() {
	if (OCA.Viewer) {
		setTimeout(function () {
			OCA.Viewer.registerHandler({
				id: 'camerarawpreviews',
				mimesAliases: {
					'image/x-dcraw': 'image/jpeg'
				}
			})
		}, 0);

	}
});

[ariselseng]

@skjnldsv
Either we should fire an event when Viewer is ready.
Or you can adjust this in beforeMount:

if(this.registeredHandlers.indexOf(handler.id) === -1) {
  this.registerHandler(handler)
}

[ariselseng]

I feel this could be easier written?

[skjnldsv]

This is meant to be a dev debug. Since no regular user would open the console I guess 🤔
But I've always been terrible at writing error messages, so feel free to suggest a simpler one 😁

[ariselseng]

@skjnldsv Hey no worries. I thought it was just unclear what it said. I am actually struggling understanding: "because to view matches". What about this: "The following file could not be displayed due to it's mime type"?

[ariselseng]

@skjnldsv
Avoiding watch.handlers before mounting fixes it too:
This is probably the cleanest fix:

diff --git a/src/views/Viewer.vue b/src/views/Viewer.vue
index 722d284..863c2d6 100644
--- a/src/views/Viewer.vue
+++ b/src/views/Viewer.vue
@@ -158,8 +158,11 @@ export default {
 
        watch: {
                // make sure any late external app can register handlers
-               handlers: function() {
-                       this.registerHandler(this.handlers[this.handlers.length - 1])
+               handlers: {
+                       handler: function() {
+                               this.registerHandler(this.handlers[this.handlers.length - 1])
+                       },
+                       immediate: false
                }
        },

[skjnldsv]

It's definitely a race condition here.

What is the error on your side then?
Is it registering too soon on your side?

[ariselseng]

@skjnldsv Yes, kind of. registerHandler is run twice, due to the watch triggering before beforeMount, I think.
The error I get is this one:

viewer/src/views/Viewer.vue

Line 330 in e46a354

console.error(`The following handler is already registered`, handler)

[ariselseng]

@skjnldsv Sorry my fix in the watch.handlers did not do anything I am afraid.
But I have determined that the watch.handlers is triggered before beforeMount DOMContentLoaded.
Does that make sense?

[ariselseng]

@skjnldsv Confirmed good fix:

diff --git a/src/views/Viewer.vue b/src/views/Viewer.vue
index 722d284..474336e 100644
--- a/src/views/Viewer.vue
+++ b/src/views/Viewer.vue
@@ -106,6 +106,7 @@ export default {
        },
 
        data: () => ({
+               loaded: false,
                handlers: OCA.Viewer.availableHandlers,
 
                components: {},
@@ -159,7 +160,9 @@ export default {
        watch: {
                // make sure any late external app can register handlers
                handlers: function() {
-                       this.registerHandler(this.handlers[this.handlers.length - 1])
+                       if (this.loaded) {
+                               this.registerHandler(this.handlers[this.handlers.length - 1])
+                       }
                }
        },
 
@@ -169,6 +172,7 @@ export default {
                        this.handlers.forEach(handler => {
                                this.registerHandler(handler)
                        })
+                       this.loaded = true
                })
 
                window.addEventListener('resize', this.onResize)

[skjnldsv]

No this is not the fix we're looking for I think. 🙈
What if an app register an action asynchronously? After the loaded is set to true?
If that is what I wanted, the solution would be easier to remove th watch 😉

The question is why does the watch gets trigered after the handler is already registered (though this is not really an issue, since it's just a warning that state that the viewer won't register the camerapreview again since it already exists)

I'd say, we should merge this and open an issue to properly investigate why the watcher gets fired despite being handled after the domReadyContent. What do you say? The current state of this pr is working for you, right? Aside from the console message?

[ariselseng]

Yes we can merge it. I am not sure what you mean with the fix. With the fix it works both asynchronously and with DOMContentLoaded. Just not asynchronously before DOMContentLoaded.

[skjnldsv]

DOMContentLoaded is mandatory anyway :)
Thanks a lot @cowai 🤗

[skjnldsv]

@cowai I ended up using your fix in #73 :)

And sorry if I sounded harsh on my previous comments. I came back to this issue and while reading them, I was not happy with the way I wrote them.
So, my apologises if you felt badly spoken to! 😕

I'm very happy of all the help and suggestions you gave me!
Thanks for being here at Nextcloud! 🤗

[ariselseng]

@skjnldsv No worries. I did not think that at all ;)
I will publish a new version of my app soon, now I know how I should integrate it to Viewer, thanks to you :)

[skjnldsv]

@cowai glad to hear!! Thanks 😉
The file_3d app is also moving to the viewer 😄 🎉