New feature: set a note as favorite (star/unstar) by korelstar · Pull Request #248 · owncloud/notes

korelstar · 2016-10-16T17:34:27Z

This pull request brings a star/unstar-feature for notes as known already from other owncloud apps: you can click on a gray star to mark a note as your favorite, then the note is shown on the top of the list and a yellow star appears, so you can easily find your most important notes.

In the technical backend, it uses the existing system to star files, i.e. a special tag for the file. The frontend provides respective new actions.

closes #202

(Open) tasks

provide setting/unsetting notes as favorite (star/unstar)
extend API for 3rd-party apps

mention-bot · 2016-10-16T17:34:28Z

@korelstar, thanks for your PR! By analyzing the history of the files in this pull request, we identified @BernhardPosselt, @Henni and @jancborchardt to be potential reviewers.

korelstar · 2016-10-16T17:35:13Z

For the Notes API, there are multiple approaches for providing this feature to 3rd-party apps, e.g.:

Introduce a new optional field favorite for creating/updating nodes, beside the existing content. Since modified should not be changed by starring a note, content have to be made optional. Then, modified could be updated if and only if a content is given, otherwise only favorite will be updated.
Introduce a new route for meta-data of a note. In the future, this could also include categories or tags. Here, it is clear, that modified will not be updated.

What do you think?

Henni

I haven't tested it yet and haven't seen the owncloud tags API in practice, but here is a pure code review for now.

Henni · 2016-10-16T18:47:09Z

+#app-navigation .active > .utils .icon-star {
+    display: inline-block;
+    opacity: .3;
+}


please merge this rule with the previous rule:

#app-navigation .active > .utils .icon-delete, #app-navigation .active > .utils .icon-star { ... }

Henni · 2016-10-16T18:48:46Z

+#app-navigation .utils .icon-starred {
+    display: inline-block;
+    opacity: 1 !important;
+}


It should be possible to remove the !important if the rule is set to:

#app-navigation .active > .utils .icon-starred {

That's not possible here, because the starred icon should be shown for not-active notes, too.

Henni · 2016-10-16T18:51:32Z

                }
            }
-        }
+        },


Where does this , come from?

copy&paste, sorry. It's removed now.

Looks good so far. Needs to be re-reviewed when finished

Henni

Except for this one issue: Awesome PR!

Henni · 2016-10-19T19:19:27Z

     * @return static
     */
-    public static function fromFile(File $file){
+    public static function fromFile(File $file, array $tags=[]){


If there are no favorites, the array annotation results in an Internal Server Error.

BTW: This is also why travis fails.

korelstar · 2016-10-19T19:21:47Z

Another thing: The automated tests failed for my pull request, because \OC::$server->getTagManager()->load('files') returns null. However, as far as I know, this is the correct official approach to get an instance of OCP\ITags (which is also used in owncloud/core: e.g., Application, Helper, TagServiceTest) and it works on my installation.

Therefore, I believe that the test environment is not sufficient. However, I have no idea how to change it, so that the tests will be successful. Can anyone help please?

jancborchardt · 2016-10-19T19:21:59Z

Really awesome work @korelstar! :) Great that it directly hooks in with Files, it’s just perfect.

Henni · 2016-10-19T19:27:37Z

     * @return static
     */
-    public static function fromFile(File $file, array $tags=[]){
+    public static function fromFile(File $file, $tags=[]){


as soon as travis is happy with this change, I'll merge it.

Thanks for you feedback. As far as I can see, this was not sufficient. See my above comment: #248 (comment)

Henni · 2016-10-19T19:42:00Z

@korelstar I haven't figured out why travis fails. Maybe you can figure it out:

Call to a member function getTagsForObjects() on a non-object in [...]/apps/notes/service/notesservice.php on line 54

https://travis-ci.org/owncloud/notes/jobs/169027138

Henni · 2016-10-19T20:00:24Z

$tagger might be null if no user is logged in.

korelstar · 2016-10-20T11:28:01Z

Which wording should be used? I've chosen favorite, because this term is used by the existing tagging system. However, star/unstar is more neutral and leaves space for other interpretations (e.g, important, to be done, interesting), so I like that wording, too. I think, I prefer this one.

What do you think? I will adjust my changes accordingly.

@jancborchardt @Henni

Henni · 2016-10-20T13:15:26Z

@korelstar I'm happy with both. @jancborchardt what do you prefer?

Henni · 2016-10-20T13:39:43Z

Two things that should be done:

Inject TagManager instead of using \OC::$server->getTagManager() (notes uses automatic dependency assembly)
Extend tests (requires dependency injection)

I'm also willing to do this. But then I would merge this PR first and fix these two points in a separate PR. (Working on two different repos is painful)

jancborchardt · 2016-10-20T13:49:32Z

Let's go with »Favorite« for now since it's symmetric to how we use it elsewhere. :)

korelstar · 2016-10-21T11:58:00Z

@Henni
That would be nice, if you could do the two tasks.
A few minutes ago, I've added the possibility to set favorites through the API (I used variant 1 of my post above: #248 (comment)). Hence, I'm ready for merge, now!

Henni · 2016-10-21T14:03:29Z

Travis finally has run through.
So 👍

New feature: set note as favorite (star/unstar)

ddf8e34

Henni suggested changes Oct 16, 2016

View reviewed changes

korelstar added 2 commits October 16, 2016 20:55

fix use of hidden classes

c55a59b

minor optimizations requested by Henni

1903317

Henni previously approved these changes Oct 16, 2016

View reviewed changes

Henni suggested changes Oct 19, 2016

View reviewed changes

remove array annotation

34dd112

Henni reviewed Oct 19, 2016

View reviewed changes

check if tagger is null

7dfe6bc

adjust tests for new favorite feature

94b5d81

allow setting favorite over API

3aed82d

Henni approved these changes Oct 21, 2016

View reviewed changes

Henni merged commit c4726b8 into owncloud:master Oct 21, 2016

korelstar deleted the favorite-feature branch October 24, 2016 11:03

korelstar mentioned this pull request Oct 25, 2016

Option to change sort method nextcloud/notes-android#92

Closed

korelstar mentioned this pull request Nov 6, 2016

New feature: set a note as favorite (star/unstar) nextcloud/notes-android#151

Merged

Uh oh!

Conversation

korelstar commented Oct 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

(Open) tasks

Uh oh!

mention-bot commented Oct 16, 2016

Uh oh!

korelstar commented Oct 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Henni left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Henni left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

korelstar commented Oct 19, 2016

Uh oh!

jancborchardt commented Oct 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Henni commented Oct 19, 2016

Uh oh!

Henni commented Oct 19, 2016

Uh oh!

korelstar commented Oct 20, 2016

Uh oh!

Henni commented Oct 20, 2016

Uh oh!

Henni commented Oct 20, 2016

Uh oh!

jancborchardt commented Oct 20, 2016

Uh oh!

korelstar commented Oct 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Henni commented Oct 21, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

korelstar commented Oct 16, 2016 •

edited

Loading

korelstar commented Oct 16, 2016 •

edited

Loading

korelstar commented Oct 21, 2016 •

edited

Loading