Public interfaces for dashboard app #10222

ArtificialOwl · 2018-07-12T16:46:20Z

No description provided.

rullzer · 2018-07-12T18:58:02Z

lib/public/Dashboard/IDashboardWidget.php

@@ -0,0 +1,53 @@
+<?php
+


Licence header

rullzer · 2018-07-12T18:58:28Z

lib/public/Dashboard/IDashboardWidget.php

+ /**
+ * @return string
+ */
+ public function getId();


NC14 is >php 7.0. So you can use return types

can I use : void ? (php 7.1)

rullzer · 2018-07-12T18:58:53Z

lib/public/Dashboard/IDashboardWidget.php

@@ -0,0 +1,53 @@
+<?php


strict typing since we are >7.0 anyway

ArtificialOwl · 2018-07-13T11:28:48Z

@rullzer: please have a look to the asked edit

nickvergessen · 2018-07-16T08:17:48Z

lib/public/Dashboard/Model/IWidgetEvent.php

+ *
+ * @param string $widgetId
+ */
+ public function __construct(string $widgetId);


I wouldn't add the constructor to the interfaces.
Just call setWidgetId instead

juliushaertl · 2018-07-17T11:51:17Z

@daita You will also need to adjust the following xsd files to make the dashboard section valid in the appinfo.xml:

https://github.com/nextcloud/server/blob/master/resources/app-info-shipped.xsd
https://github.com/nextcloud/server/blob/master/resources/app-info.xsd

And the app store needs those updates as well.

juliushaertl · 2018-07-17T11:55:55Z

Just a general thing, when we add new sections to the appinfo.xml like for this case, does this break apps that also support versions previous to 14?

The app store only has one version-independent file to validate the appinfo, so that should work fine, but does the server also checks for a valid appinfo at some point?

nickvergessen · 2018-07-17T14:44:14Z

Yes the app check-code checks it, but additional stuff is not a problem as far as I know.
Only if you have stuff, which is not parsing valid, it will cry

MorrisJobke · 2018-07-17T21:57:01Z

lib/public/Dashboard/IDashboardWidget.php

+interface IDashboardWidget {
+
+ /**
+ * @return string


Missing @since 14.0.0 on the methods and the interface.

MorrisJobke · 2018-07-17T21:57:23Z

lib/public/Dashboard/IDashboardWidget.php

@@ -0,0 +1,79 @@
+<?php declare(strict_types=1);


Put the declare() on a new line.

MorrisJobke · 2018-07-17T22:00:13Z

It's a bit late for that ... freeze was on Friday ... at least for the server PR.

And also it was never brought up to be needed for 14. :/

jancborchardt · 2018-07-17T22:06:46Z

No need to rush in the dashboard, no? We can merge it very early for 15, and then properly work on adding endpoints to the different apps and have the Dashboard app use those, like any other app.

ArtificialOwl · 2018-07-17T22:12:46Z

Well, you know. After working on something else for months and finally having few days to do something fun. But I get it; @nickvergessen already told me last week that I should not waste my time on it.

So, no rush.

ArtificialOwl · 2018-09-01T17:05:28Z

So, ready for NC15.

@nickvergessen : Added also a IDashboardManager that can be called with Automatic DI so there is no more API, just call the right method from the dashboardManager, that will check if app is installed or not.

nickvergessen · 2018-09-03T12:46:29Z

lib/private/Dashboard/DashboardManager.php

+ * @throws DashboardAppNotAvailableException
+ */
+ public function createGlobalEvent(string $widgetId, array $payload, string $uniqueId = ''
+ ) {


nickvergessen · 2018-09-03T12:47:59Z

resources/app-info-shipped.xsd

+ maxOccurs="unbounded"/>
+ maxOccurs="unbounded"/>
+ </xs:sequence>
+ </xs:complexType>


this needs to go to the appstore too

More general why is this needed here? I don't see it being used yet.

ChristophWurst

Some nitpicks. Please also make sure you have a line break at the end of each file (as github shows here) to be consistent with our other source files.

Thanks!

ChristophWurst · 2018-09-05T15:53:56Z

lib/public/Dashboard/IDashboardWidget.php

+ /**
+ * @since 15.0.0
+ *
+ * @return array


Please add more documentation to these methods. What are they supposed to return? array is very generic 😉

ChristophWurst · 2018-09-05T15:54:05Z

lib/public/Dashboard/IDashboardWidget.php

+ /**
+ * @since 15.0.0
+ *
+ * @return array


same as above

ChristophWurst · 2018-09-05T15:55:26Z

lib/public/Dashboard/Model/IWidgetRequest.php

+ *
+ * @param string $widgetId
+ */
+ public function __construct(string $widgetId);


the interface determines the constructor interface? This doesn't seem right 🙈

ChristophWurst · 2018-09-05T15:56:01Z

lib/public/Dashboard/Model/IWidgetRequest.php

+ /**
+ * @since 15.0.0
+ *
+ * @param string $request


what kind of request is this string?

@daita 🏓

🤷‍♂️ 🏓

ChristophWurst · 2018-09-05T15:56:20Z

lib/public/Dashboard/Model/IWidgetSettings.php

+ * @param string $userId
+ * @param string $widgetId
+ */
+ public function __construct(string $widgetId, string $userId);


Please remove this. Same issue as above.

@daita 🏓

saw that ... removed.

rullzer · 2018-09-06T19:55:12Z

resources/app-info.xsd

+ <xs:sequence>
+ <xs:element name="widget" type="php-class" minOccurs="1"
+ maxOccurs="unbounded"/>
+ maxOccurs="unbounded"/>


that doesn't look right.

rullzer · 2018-09-06T19:55:38Z

resources/app-info-shipped.xsd

+ <xs:sequence>
+ <xs:element name="widget" type="php-class" minOccurs="1"
+ maxOccurs="unbounded"/>
+ maxOccurs="unbounded"/>


nickvergessen · 2018-09-13T07:35:00Z

lib/private/Dashboard/DashboardManager.php

+ }
+
+ try {
+ return \OC::$server->query($service);


you can inject IServerContainer in your constructor. Then you can also better unit test this.

ArtificialOwl · 2018-10-04T07:55:09Z

🏓

Would be great to go on with this one, as it is a blocker to work on the dev of widgets for 15.
(I know it's missing tests, this would come later)

MorrisJobke · 2018-10-08T07:18:05Z

lib/public/Dashboard/IDashboardWidget.php

+ * 'content' => (string) path to the HTML Template of the widget,
+ * 'function' => (string) JavaScript function to be called when
+ * loading the widget on the dashboard
+ * ];


A template object would be nice, because it is automatically properly typed and has then proper autocompletion as well. 😉

moved to objects in various spot, pushed. Will comment during the day.

MorrisJobke · 2018-10-08T07:18:53Z

lib/public/Dashboard/IDashboardWidget.php

+ * ]
+ *
+ * 'push' contains the JS function to be called on push event:
+ * 'push' => (string) JavaScript function to be called on push event


A settings object would be nice instead of a multidimensional array. 🤔

MorrisJobke

Beside the comments it looks good 👍

Mostly addressed

ChristophWurst · 2018-10-10T08:32:33Z

lib/private/Dashboard/DashboardManager.php

+ */
+ public function getWidgetConfig(string $widgetId, string $userId): IWidgetConfig {
+ /** @var \OCA\Dashboard\Service\WidgetsService $widgetsService */
+ $widgetsService = $this->getService('\OCA\Dashboard\Service\WidgetsService');


please use the ::class syntax, this makes finding class usages and refactoring later a lot easier 👍

Would require another try-catch in here of course, in case the dashboard app is not here.

Why would it? ::class is simply replaced by the FQCN (string) by the interpreter.

I thought this would invoke autoloading and therefor throw? 🤷

The thing is that I am not sure the app is available at this point, and calling the ::class does not clean enough

$ php -a Interactive shell php > echo Joas::class; Joas

ChristophWurst · 2018-10-10T08:32:42Z

lib/private/Dashboard/DashboardManager.php

+ */
+ public function createUsersEvent(string $widgetId, array $users, array $payload, string $uniqueId = '') {
+ /** @var \OCA\Dashboard\Service\EventsService $eventsService */
+ $eventsService = $this->getService('\OCA\Dashboard\Service\EventsService');


ChristophWurst · 2018-10-10T08:32:48Z

lib/private/Dashboard/DashboardManager.php

+ */
+ public function createGroupsEvent(string $widgetId, array $groups, array $payload, string $uniqueId = '') {
+ /** @var \OCA\Dashboard\Service\EventsService $eventsService */
+ $eventsService = $this->getService('\OCA\Dashboard\Service\EventsService');


ChristophWurst · 2018-10-10T08:32:54Z

lib/private/Dashboard/DashboardManager.php

+ */
+ public function createGlobalEvent(string $widgetId, array $payload, string $uniqueId = '') {
+ /** @var \OCA\Dashboard\Service\EventsService $eventsService */
+ $eventsService = $this->getService('\OCA\Dashboard\Service\EventsService');


ChristophWurst · 2018-10-10T08:35:49Z

lib/public/Dashboard/Model/IWidgetRequest.php

+ /**
+ * @since 15.0.0
+ *
+ * @param string $request


🤷‍♂️ 🏓

ChristophWurst · 2018-10-10T08:36:40Z

lib/public/Dashboard/Model/IWidgetTemplate.php

+ public function getInitFunction(): string;
+
+ /**
+ * JavaScript function to be called when loading the widget on the


how will that function be invoked? Does it have to exist globally?

it does not have to exist globally, it is called by the app

ChristophWurst · 2018-10-10T08:37:12Z

lib/private/Dashboard/Model/WidgetTemplate.php

+ *
+ * @return array
+ */
+ public function getCss(): array {


how will this CSS be loaded? Is this property the actual CSS contents or just the URL?

rullzer

I don't see a reason for much of this implementation to be in the server. Like the actual implementation of a lot of those classes can just be in the dashboard app itself right?

rullzer · 2018-10-10T10:33:49Z

lib/private/Dashboard/Model/WidgetSetting.php

+ *
+ * @param string $type
+ */
+ public function __construct($type = '') {


rullzer · 2018-10-10T10:38:28Z

resources/app-info-shipped.xsd

+ maxOccurs="unbounded"/>
+ maxOccurs="unbounded"/>
+ </xs:sequence>
+ </xs:complexType>


More general why is this needed here? I don't see it being used yet.

lib/public/Dashboard/Service/IEventsService.php

ChristophWurst

Looks a lot better 👍

Let's move the implementations to the dashboard app. I don't think they should be in the server as they aren't even used directly.

Nitpick: @see WidgetSetting is a bit misleading. Not sure what it should tell me. It's also a bit misplaced in the interfaces because that would point to the private implementation. All usage should be documented in the public interfaces though 😉

lib/private/Dashboard/DashboardManager.php

ChristophWurst · 2018-10-12T06:51:31Z

lib/private/Dashboard/Model/WidgetSetting.php

+ *
+ * @package OC\Dashboard\Model
+ */
+class WidgetSetting implements JsonSerializable {


I think this implementation can be safely move to the dashboard app

ChristophWurst · 2018-10-12T06:52:40Z

lib/private/Dashboard/Model/WidgetSetup.php

+ *
+ * @package OC\Dashboard\Model
+ */
+class WidgetSetup implements JsonSerializable {


I think this implementation can be safely move to the dashboard app

ChristophWurst · 2018-10-12T06:52:51Z

lib/private/Dashboard/Model/WidgetTemplate.php

+ *
+ * @package OC\Dashboard\Model
+ */
+class WidgetTemplate implements JsonSerializable {


I think this implementation can be safely move to the dashboard app

ArtificialOwl · 2018-10-12T08:06:37Z

@ChristophWurst

I removed the unused variable from DashboardManager
WidgetSetting, WidgetSetup and WidgetTemplate are created by the app that generate the widget, so it should stays here. This should be documented in the IDashboardWidget

ChristophWurst · 2018-10-12T08:16:05Z

WidgetSetting, WidgetSetup and WidgetTemplate are created by the app that generate the widget, so it should stays here. This should be documented in the IDashboardWidget

WidgetSettings is not used outside the private namespace. Hence it must not be used by apps.

IDashboardWidget::getWidgetSetup returns a WidgetSetup, which violates the public/private API separation. It should instead return an IWidgetSetup. Fixing this will effectively make the WidgetSetup implementation independent of server and apps, hence moving it to the dashboard app is fine/recommended.

ChristophWurst · 2018-10-12T08:20:17Z

* WidgetSetting, WidgetSetup and WidgetTemplate are created by the app that generate the widget, so it should stays here. This should be documented in the IDashboardWidget

If a construct (class/interface) is used by an app, it has to be a public API. There are ~~two~~ three possible use cases:

The server only cares about an objects methods: create an interface that is implemented in an app's class.
The app should extend an existing (potentially abstract) class: export the class in the public API
The app use objects generated by the server or dashboard app: extract an interface from those interfaces, export them in the public API and provide factory classes/methods to create the objects as public API.

ChristophWurst · 2018-10-12T11:14:20Z

After a bit of discussion I think I've finally understood how the different components will be used. I created a little diagram that outlines where certain classes/interfaces live:

This works for @daita and clearly separates what should be in server/dashboard and other apps. This removes and dependency from server on the dashboard app. The implementation part in server (esp the private part) is minimal. The three classes for setup, settings and template were deliberately moved to OCP as classes as they are simple models (or better data transfer objects).

rullzer · 2018-10-12T14:37:47Z

After a bit of discussion I think I've finally understood how the different components will be used. I created a little diagram that outlines where certain classes/interfaces live:

This works for @daita and clearly separates what should be in server/dashboard and other apps. This removes and dependency from server on the dashboard app. The implementation part in server (esp the private part) is minimal. The three classes for setup, settings and template were deliberately moved to OCP as classes as they are simple models (or better data transfer objects).

Yes makes sense.
As some of the classes are only tiny and there to transport data. I would like to make the getters and setters at least final. We can always losen this later. But lets start from a strict implementation.

rullzer

Ok I think it is time to get this in.
It looks sane to me.

Lets see how far we get.
@daita could you squash your commits?

ChristophWurst

Wasn't the plan to have WidgetSettings, WidgetSetup and WidgetTemplate in OCP? They're in the private namespace right now.

ChristophWurst

Looks good now!

You probably have to run ./build/autoloaderchecker.sh to make CI happy.

Also, please make squash this and make this a single commit change 😉

MorrisJobke · 2018-10-15T16:40:25Z

CI says no: https://drone.nextcloud.com/nextcloud/server/11524/93

ChristophWurst

Please remove additional files and rerun autoloader command

ChristophWurst · 2018-10-15T17:00:54Z

lib/composer/composer/autoload_classmap.php

+ 'OCP\\Dashboard\\IDashboardManager' => $baseDir . '/lib/public/Dashboard/IDashboardManager.php',
+ 'OCP\\Dashboard\\IDashboardWidget' => $baseDir . '/lib/public/Dashboard/IDashboardWidget.php',
+ 'OCP\\Dashboard\\Model\\IWidgetConfig' => $baseDir . '/lib/public/Dashboard/Model/IWidgetConfig.php',
+ 'OCP\\Dashboard\\Model\\IWidgetRequest' => $baseDir . '/lib/public/Dashboard/Model/IWidgetRequest.php',


interfaces were removed

ChristophWurst

Nvm the last review, I was confused by the interface names it seems

@SInCE

Signed-off-by: Maxence Lange <maxence@artificial-owl.com> Merge remote-tracking branch 'origin/interface-dashboard' into interface-dashboard Signed-off-by: Maxence Lange <maxence@artificial-owl.com> moving data object to OCP Signed-off-by: Maxence Lange <maxence@artificial-owl.com> update autoload files Signed-off-by: Maxence Lange <maxence@artificial-owl.com> +@SInCE Signed-off-by: Maxence Lange <maxence@artificial-owl.com>

ArtificialOwl added the 3. to review Waiting for reviews label Jul 12, 2018

rullzer previously requested changes Jul 12, 2018

View reviewed changes

nickvergessen reviewed Jul 16, 2018

View reviewed changes

ArtificialOwl mentioned this pull request Jul 17, 2018

+ dashboard/widget nextcloud/appstore#567

Merged

MorrisJobke reviewed Jul 17, 2018

View reviewed changes

MorrisJobke added this to the Nextcloud 15 milestone Jul 17, 2018

juliushaertl mentioned this pull request Jul 25, 2018

framework for deck widgets nextcloud/deck#540

Closed

nickvergessen reviewed Sep 3, 2018

View reviewed changes

ChristophWurst previously requested changes Sep 5, 2018

View reviewed changes

rullzer reviewed Sep 6, 2018

View reviewed changes

nickvergessen reviewed Sep 13, 2018

View reviewed changes

MorrisJobke added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Sep 25, 2018

MorrisJobke added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Oct 8, 2018

MorrisJobke reviewed Oct 8, 2018

View reviewed changes

ArtificialOwl added 2. developing Work in progress 3. to review Waiting for reviews and removed 3. to review Waiting for reviews 2. developing Work in progress labels Oct 9, 2018

This comment has been minimized.

Sign in to view

ChristophWurst requested changes Oct 10, 2018

View reviewed changes

rullzer reviewed Oct 10, 2018

View reviewed changes

This comment has been minimized.

Sign in to view

ChristophWurst reviewed Oct 10, 2018

View reviewed changes

lib/public/Dashboard/Service/IEventsService.php Outdated Show resolved Hide resolved

ChristophWurst requested changes Oct 12, 2018

View reviewed changes

rullzer approved these changes Oct 15, 2018

View reviewed changes

ChristophWurst requested changes Oct 15, 2018

View reviewed changes

ChristophWurst approved these changes Oct 15, 2018

View reviewed changes

ArtificialOwl force-pushed the interface-dashboard branch 2 times, most recently from 3689505 to 323c330 Compare October 15, 2018 16:01

ChristophWurst requested changes Oct 15, 2018

View reviewed changes

ChristophWurst approved these changes Oct 15, 2018

View reviewed changes

ArtificialOwl force-pushed the interface-dashboard branch from 0880b33 to b98fa11 Compare October 15, 2018 17:25

rullzer force-pushed the interface-dashboard branch from b98fa11 to dfd4782 Compare October 15, 2018 18:00

ArtificialOwl merged commit f5a495b into master Oct 15, 2018

ArtificialOwl deleted the interface-dashboard branch October 15, 2018 18:56

Public interfaces for dashboard app #10222

Public interfaces for dashboard app #10222

Conversation

ArtificialOwl commented Jul 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArtificialOwl commented Jul 13, 2018

Choose a reason for hiding this comment

juliushaertl commented Jul 17, 2018 • edited Loading

juliushaertl commented Jul 17, 2018

nickvergessen commented Jul 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MorrisJobke commented Jul 17, 2018

jancborchardt commented Jul 17, 2018

ArtificialOwl commented Jul 17, 2018

ArtificialOwl commented Sep 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChristophWurst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArtificialOwl commented Oct 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MorrisJobke left a comment

Choose a reason for hiding this comment

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rullzer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

ChristophWurst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArtificialOwl commented Oct 12, 2018

ChristophWurst commented Oct 12, 2018

ChristophWurst commented Oct 12, 2018 • edited Loading

ChristophWurst commented Oct 12, 2018

rullzer commented Oct 12, 2018

rullzer left a comment

Choose a reason for hiding this comment

ChristophWurst left a comment

Choose a reason for hiding this comment

ChristophWurst left a comment

Choose a reason for hiding this comment

MorrisJobke commented Oct 15, 2018

ChristophWurst left a comment

juliushaertl commented Jul 17, 2018 •

edited

Loading

ChristophWurst commented Oct 12, 2018 •

edited

Loading