API design questions/comments

[nickvergessen]

Remove Widget knowledge / App dependency

The Dashboard should not need to know names, services or anything about the widget apps. Same goes for the other way around. Instead there should be an event, which apps can listen to. I can post you some demo code if you need it.

Remove the joker

/* 
*optional*, default: nothing
A "Joker" for later use... maybe...
*/
$widget['special']                  = '';

Remove JS/CSS stuff

/*
*optional* default: empty
provide the pathes to aditional cascading style sheets as an array. 
*/
$widget['css']                      = array('one file');

/*
*optional* default: empty
provide the pathes to your javascript files as an array. . 
*/
$widget['js']                       = array('one file', 'another file');

Since you are in an event anyway, and the event is only called in the dashboard controller, so when visiting the dashboard, apps can immediately load their files. Nothing the dashboard should take care of

If you have questions about this, just mention me and I will help you :smiley:

[dehnhardt]

@nickvergessen Wow, thanks for your comments, Joas - and: I definitely need some more information:

The Dashboard should not need to know names, services or anything about the widget apps. Same goes for the other way around.

Does this mean there should be no api call at all in the Application.php but everything in an event listener? Some example code really might be very helpful!

so when visiting the dashboard, apps can immediately load their files

How can I do that? I'm sure it will keep me - eh the dashboard - from a lot of trouble.

If you have questions about this, just mention me and I will help you

Again: Thank you for your offer, every help is very welcome!

Holger

[nickvergessen]

Okay, I will follow up with examples, later this week

[dehnhardt]

so when visiting the dashboard, apps can immediately load their files

How can I do that? I'm sure it will keep me - eh the dashboard - from a lot of trouble.

Ah, you mean, loading the files in the "widget template class" of the providing app - right?

[nickvergessen]

Or in the listener directly, but yes the widget app takes care of that, instead of the dashboard

[dehnhardt]

Thanks, so I can start modifiying his evening;-)

[dehnhardt]

Just a little update: Yesterday I tried to move the including of javascript and css files to the apps, that provide the widgets - but so far with no success. Presumably because the header section of the page is constructed before the addScript/addStyle methods are called. I will investigate this further tonight.

[nickvergessen]

Okay, I tried to have a look and didn't really understand the structure, so I couldn't manage to get that working either. However I guess the part about "listener/events" is the more important part. The css/js loading would have been nice/better. But the other point is violating basic principles.

[dehnhardt]

@nickvergessen

Okay, I tried to have a look and didn't really understand the structure, so I couldn't manage to get that working either.

The same problem here;-) I insert a lot of debug statements but still I'm not familiar with most of the structure.

However I guess the part about "listener/events" is the more important part.

Here it would be nice if you could point me to some examples or existing code.

[dehnhardt]

@nickvergessen I currently have some time to rewrite the api. If I understand your remarks, the problem is, that the applications, which provide an widget for the dashboard should not need any class from the dashboar app because the dashboard app must not be present? Instead the dashboard app should sent out an event which other apps can listen to and provide their widget inside the event. But how can I do that? There is nothing in the docs (beside the existing hooks). It reaaly would be nice if you can point me to som examples give some hints.

Holger

[nickvergessen]

Ah, now I just replied on your API topic in the forum. Would be nice if you can push your changes and create a PR for it, then I can comment a bit closer on it.

[dehnhardt]

Hi Joas, thanks for coming back to this issue. Unfortunately my time off work has ended now, so next steps will be "nightwork" again. As I did not get any further with my modifications, I did not commit anything. I will create a branch and check in the changes there. As well I will try to make some kind of activity diagram to show, what I will accomplish. It might help to discuss possible solution.

[dehnhardt]

@nickvergessen Dear Joas, I have added a branch (API2) with a few changes: An commented out call to dispatch in two files: appinfo/Application.php (line 70) and controller/routepagecontroller.php (line 46) I also have added a second repository "widgets" with a default app, craeted be ncdev startapp. (Later I will move the default widgets to this app) I changed nothig but the lines 52 to 59, where I added a listener to the dashboard event dispatch.

If the listener and one of the the dispatcher is activated, the application seems to be in some kind of loop when I try to open the dashboard. When either the dispatch of the event or the listen to the event is commented out, the dashboard loads with no problems. I can't see any reason for this behaviour.

Holger

[dehnhardt]

[nickvergessen]

Well the problem is, that you $this->dispatcher->dispatch('OCA\Dashboard\RequestWidget'); in your application class. But your application class is loaded from app.php, since dashboard is before widgets in the alphabet, the widget app had no chance to register their widgets in it's app.php before they where grabbed.

Basically you should try to delay $this->dispatcher->dispatch('OCA\Dashboard\RequestWidget'); as long as possible. You only need it when you are in your controller, because if you don't end up there, you don't need it. So move that line to the controller's index method.

At least that is what my quick view gave me, if that doesn't help I can have a closer look this afternoon

[dehnhardt]

Basically you should try to delay $this->dispatcher-> dispatch('OCA\Dashboard\RequestWidget'); as long as possible.

I did try it as well in the index method...

An commented out call to dispatch in two files: appinfo/Application.php (line 70) and controller/routepagecontroller.php (line 46)

...with the same result.

But at least I know now, that Application is a bad place for the dispatch;-) If you have time to take a closer look in the next days, I really would appreciate it!

[nickvergessen]

Will try to have a look this afternoon.

[dehnhardt]

I get a little further: The event callback does nothing more than logging one line:

$listener = function ($event) {
    \OCP\Util::writeLog( 'Widgets', 'Widtgest callwidgets ' . print_r( $event, true ), \OCP\Util::DEBUG );
    ;
};

and this seems to be the problem:-( When commenting out the writeLog Statement, the application continues. Could it be, that logging is implemented through events as well and that this causes the problems?

[nickvergessen]

https://github.com/dehnhardt/nc_dashboard/blob/API2/appinfo/app.php#L50 => \OCP\App otherwise class is not found in NC 10.

Widgets repo does not match the name of the appid. Also I'd go with one app/repo per widget, to make it a little more explicit and to keep the "noice" low when you want to help out on one widget. We experienced this with the apps/ repo in owncloud/nextcloud and since then switched to one repo per app.

Also not sure, but the print_r seems to kill it. IF I replace the listener in the widgets app with:

$listener = function ($event) {
    \OCP\Util::writeLog( 'Widgets', 'Widgets callwidgets', \OCP\Util::DEBUG );
};

It logs just fine for me:

{"reqId":"bW6cQ3qfH0OO6MJxX1Xd","remoteAddr":"::1","app":"dashboard","message":"**** call: index","level":0,"time":"2016-08-26T13:09:31+00:00","method":"GET","url":"\/index.php\/apps\/dashboard\/","user":"admin"}
{"reqId":"bW6cQ3qfH0OO6MJxX1Xd","remoteAddr":"::1","app":"Dashboard","message":"Dashboard: Before dispatch","level":0,"time":"2016-08-26T13:09:32+00:00","method":"GET","url":"\/index.php\/apps\/dashboard\/","user":"admin"}
{"reqId":"bW6cQ3qfH0OO6MJxX1Xd","remoteAddr":"::1","app":"Widgets","message":"Widgets callwidgets","level":0,"time":"2016-08-26T13:09:32+00:00","method":"GET","url":"\/index.php\/apps\/dashboard\/","user":"admin"}
{"reqId":"bW6cQ3qfH0OO6MJxX1Xd","remoteAddr":"::1","app":"Dashboard","message":"Dashboard: After dispatch","level":0,"time":"2016-08-26T13:09:32+00:00","method":"GET","url":"\/index.php\/apps\/dashboard\/","user":"admin"}

Or did I misunderstand your question?

Sorry for taking again so long, but the NC 10 release from yesterday just ate all my time.

[dehnhardt]

First of all: No reason to apologize. I really appreciate your help! You are right, the problem is the print_r... for whatever reason. Now the callback is executed without any problem.

https://github.com/dehnhardt/nc_dashboard/blob/API2/appinfo/app.php#L50 => \OCP\App otherwise class is not found in NC 10.

Ok, locally fixed. Will commit it later. One app per repo is of course a good idea and the difference between repo name and app name is just because of me still not being familar with git... One widget per app is not my intention, as I don't want to limit an app developer if he wants to provide more than one for his app.