Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add favorites to files app #12360

Merged
merged 10 commits into from
Dec 15, 2014
Merged

Add favorites to files app #12360

merged 10 commits into from
Dec 15, 2014

Conversation

PVince81
Copy link
Contributor

Part of #2368

},
type: 'GET',
beforeSend: function(xhr) {
xhr.setRequestHeader('OCS-APIREQUEST', 'true');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Killed with #12362

@PVince81
Copy link
Contributor Author

@PVince81
Copy link
Contributor Author

Favorites kind of work now.

I used a horrendous approach to populate the tags inside the result: 5ea8c8c#diff-471be045dd1eb605d9a1197b9ba90560R51
@icewind1991 can you comment on this ? Any better ideas ? (apart from bundling the ids together)

  • Fix layout
  • Fix potential performance bottleneck
  • Submit ITags addition as separate PR (45ac3d3) + unit tests
  • Make "Favorites" list work as well

@PVince81
Copy link
Contributor Author

@PVince81 PVince81 added this to the 8.0-current milestone Nov 25, 2014
@PVince81
Copy link
Contributor Author

  • update docs

* @param array $tags array of tags
*/
public function updateFileMetadata($path, $tags) {
$view = new \OC\Files\View('/'.\OCP\User::getUser().'/files');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't use private API. If there is no public API for this: create it ;)

@PVince81
Copy link
Contributor Author

  • to fix tags/proxy perf potential issue: look into doing the JOIN directly built-in in the FS layer instead of making it pluggable

@PVince81
Copy link
Contributor Author

After discussion with @karlitschek (#2368 (comment)), I moved the tags code into a separate app here: owncloud-archive/metadata#2

I'll keep this PR here to make the necessary API/hook/proxy changes in core to make that app work.

@PVince81 PVince81 changed the title [WIP] Favorites file list [WIP] Core changes for tags/favorites for the metadata app Nov 28, 2014
@PVince81 PVince81 force-pushed the files-tags branch 2 times, most recently from bedc636 to 3624130 Compare December 1, 2014 16:57
@PVince81
Copy link
Contributor Author

PVince81 commented Dec 8, 2014

Talked with @icewind1991 and we decided to move the tags injection to the presentation layer (ajax/list.php) to prevent potential performance issues when parts of the code or apps are calling getDirectoryContents or getFileInfo even when they don't need the tags. Those parts of the code can always use the ITags handler to retrieve tags when needed.

@PVince81 PVince81 changed the title [WIP] Core changes for tags/favorites for the metadata app Add favorites to files app Dec 11, 2014
@PVince81
Copy link
Contributor Author

@jancborchardt I removed the spinner as you asked, please try again.

@ghost
Copy link

ghost commented Dec 15, 2014

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/4539/

Build result: FAILURE

[...truncated 16 lines...]Fetching upstream changes from https://github.com/owncloud/core.gitusing GIT_SSH to set credentials using .gitcredentials to set credentials > git config --local credential.helper store --file=/tmp/git5588186876880144167.credentials # timeout=10 > git fetch --tags --progress https://github.com/owncloud/core.git +refs/pull/:refs/remotes/origin/pr/ > git config --local --remove-section credential # timeout=10 > git rev-parse origin/pr/12360/merge^{commit} # timeout=10 > git branch -a --contains cb5c32ade0d4fcb03e9309c09aba8dbf845a2ab7 # timeout=10 > git rev-parse remotes/origin/pr/12360/merge^{commit} # timeout=10Checking out Revision cb5c32ade0d4fcb03e9309c09aba8dbf845a2ab7 (origin/pr/12360/merge) > git config core.sparsecheckout # timeout=10 > git checkout -f cb5c32ade0d4fcb03e9309c09aba8dbf845a2ab7First time build. Skipping changelog. > git remote # timeout=10 > git submodule init # timeout=10 > git submodule sync # timeout=10 > git config --get remote.origin.url # timeout=10 > git submodule update --init --recursiveTriggering pull-request-analyser-ng-simple » SLAVEpull-request-analyser-ng-simple » SLAVE completed with result FAILUREStarted calculate disk usage of buildFinished Calculation of disk usage of build in 0 secondsStarted calculate disk usage of workspaceFinished Calculation of disk usage of workspace in 3 second
💣 Test FAILed. 💣

@ghost
Copy link

ghost commented Dec 15, 2014

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/4541/

Build result: FAILURE

[...truncated 17 lines...]using GIT_SSH to set credentials using .gitcredentials to set credentials > git config --local credential.helper store --file=/tmp/git7205812824551924239.credentials # timeout=10 > git fetch --tags --progress https://github.com/owncloud/core.git +refs/pull/:refs/remotes/origin/pr/ > git config --local --remove-section credential # timeout=10 > git rev-parse origin/pr/12360/merge^{commit} # timeout=10 > git branch -a --contains e4050c3adab18bb3e5697910cf809fe88c34ff73 # timeout=10 > git rev-parse remotes/origin/pr/12360/merge^{commit} # timeout=10Checking out Revision e4050c3adab18bb3e5697910cf809fe88c34ff73 (origin/pr/12360/merge) > git config core.sparsecheckout # timeout=10 > git checkout -f e4050c3adab18bb3e5697910cf809fe88c34ff73 > git rev-list 9075db939b68cfc6cf3cc07061c51624cec5acb4 # timeout=10First time build. Skipping changelog. > git remote # timeout=10 > git submodule init # timeout=10 > git submodule sync # timeout=10 > git config --get remote.origin.url # timeout=10 > git submodule update --init --recursiveTriggering pull-request-analyser-ng-simple » SLAVEpull-request-analyser-ng-simple » SLAVE completed with result FAILUREStarted calculate disk usage of buildFinished Calculation of disk usage of build in 0 secondsStarted calculate disk usage of workspaceFinished Calculation of disk usage of workspace in 2 second
💣 Test FAILed. 💣

@ghost
Copy link

ghost commented Dec 15, 2014

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/4546/

Build result: FAILURE

[...truncated 17 lines...]using GIT_SSH to set credentials using .gitcredentials to set credentials > git config --local credential.helper store --file=/tmp/git7644682796774668943.credentials # timeout=10 > git fetch --tags --progress https://github.com/owncloud/core.git +refs/pull/:refs/remotes/origin/pr/ > git config --local --remove-section credential # timeout=10 > git rev-parse origin/pr/12360/merge^{commit} # timeout=10 > git branch -a --contains b87d02551002cfded4d501cc3d2e2c4c7b69dcd2 # timeout=10 > git rev-parse remotes/origin/pr/12360/merge^{commit} # timeout=10Checking out Revision b87d02551002cfded4d501cc3d2e2c4c7b69dcd2 (origin/pr/12360/merge) > git config core.sparsecheckout # timeout=10 > git checkout -f b87d02551002cfded4d501cc3d2e2c4c7b69dcd2 > git rev-list 9075db939b68cfc6cf3cc07061c51624cec5acb4 # timeout=10First time build. Skipping changelog. > git remote # timeout=10 > git submodule init # timeout=10 > git submodule sync # timeout=10 > git config --get remote.origin.url # timeout=10 > git submodule update --init --recursiveTriggering pull-request-analyser-ng-simple » SLAVEpull-request-analyser-ng-simple » SLAVE completed with result FAILUREStarted calculate disk usage of buildFinished Calculation of disk usage of build in 0 secondsStarted calculate disk usage of workspaceFinished Calculation of disk usage of workspace in 2 second
💣 Test FAILed. 💣

@jancborchardt
Copy link
Member

Works nicely! 👍

@jancborchardt
Copy link
Member

@PVince81 what’s up with the Jenkins failures? Otherwise it seems good to go.

@PVince81
Copy link
Contributor Author

Thanks. Jenkins is sick recently, I'll push the PR again for the tests.

Would be good if someone could review the code.
@icewind1991 @MorrisJobke @LukasReschke ?

@PVince81
Copy link
Contributor Author

Jenkins test here: #12859


/**
* Core
*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tabs + Spaces

@PVince81
Copy link
Contributor Author

Fixed in 207d77e

@scrutinizer-notifier
Copy link

The inspection completed: 16 new issues, 13 updated code elements

@ghost
Copy link

ghost commented Dec 15, 2014

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/4554/

Build result: FAILURE

[...truncated 16 lines...]Fetching upstream changes from https://github.com/owncloud/core.gitusing GIT_SSH to set credentials using .gitcredentials to set credentials > git config --local credential.helper store --file=/tmp/git8428475028823098557.credentials # timeout=10 > git fetch --tags --progress https://github.com/owncloud/core.git +refs/pull/:refs/remotes/origin/pr/ > git config --local --remove-section credential # timeout=10 > git rev-parse origin/pr/12360/merge^{commit} # timeout=10 > git branch -a --contains 54e21ef77770c1b44f19bdb6185c21bc5eb36ba2 # timeout=10 > git rev-parse remotes/origin/pr/12360/merge^{commit} # timeout=10Checking out Revision 54e21ef77770c1b44f19bdb6185c21bc5eb36ba2 (origin/pr/12360/merge) > git config core.sparsecheckout # timeout=10 > git checkout -f 54e21ef77770c1b44f19bdb6185c21bc5eb36ba2First time build. Skipping changelog. > git remote # timeout=10 > git submodule init # timeout=10 > git submodule sync # timeout=10 > git config --get remote.origin.url # timeout=10 > git submodule update --init --recursiveTriggering pull-request-analyser-ng-simple » SLAVEpull-request-analyser-ng-simple » SLAVE completed with result FAILUREStarted calculate disk usage of buildFinished Calculation of disk usage of build in 0 secondsStarted calculate disk usage of workspaceFinished Calculation of disk usage of workspace in 1 second
💣 Test FAILed. 💣

@LukasReschke
Copy link
Member

👍

LukasReschke added a commit that referenced this pull request Dec 15, 2014
@LukasReschke LukasReschke merged commit be3d4fd into master Dec 15, 2014
@LukasReschke LukasReschke deleted the files-tags branch December 15, 2014 18:55
@lock lock bot locked as resolved and limited conversation to collaborators Aug 15, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants