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

↔️ Define and implement strategy for persisting order of apps in top bar. (Database?) #40254

Closed
Tracked by #40241
sorbaugh opened this issue Sep 4, 2023 · 6 comments

Comments

@sorbaugh
Copy link
Contributor

sorbaugh commented Sep 4, 2023

subtask to #40241

@sorbaugh sorbaugh moved this to 📄 To do (~10 entries) in 📁 Files team Sep 4, 2023
@sorbaugh sorbaugh added this to the Nextcloud 28 milestone Sep 4, 2023
@AndyScherzinger AndyScherzinger changed the title Define and implement strategy for persisting. (Database?) ↔️ Define and implement strategy for persisting. (Database?) Sep 5, 2023
@sorbaugh sorbaugh changed the title ↔️ Define and implement strategy for persisting. (Database?) ↔️ Define and implement strategy for persisting order of apps in top bar. (Database?) Sep 5, 2023
@sorbaugh sorbaugh assigned sorbaugh and come-nc and unassigned sorbaugh Sep 7, 2023
@come-nc
Copy link
Contributor

come-nc commented Sep 19, 2023

Pointers:

Order is stored in info.xml for each app, in navigations/navigation/order.
The method AppManager::getAppInfo is used to get information about each application. I think it would be the right place to alter the value of order for each navigation entry.
https://github.com/nextcloud/server/blob/master/lib/private/App/AppManager.php#L710

@come-nc
Copy link
Contributor

come-nc commented Sep 19, 2023

POC:

diff --git a/lib/private/NavigationManager.php b/lib/private/NavigationManager.php
index ac6f16190dc..31f6b74691a 100644
--- a/lib/private/NavigationManager.php
+++ b/lib/private/NavigationManager.php
@@ -290,6 +290,7 @@ class NavigationManager implements INavigationManager {
                        $apps = $this->appManager->getInstalledApps();
                }
 
+               $user = $this->userSession->getUser();
                foreach ($apps as $app) {
                        if (!$this->userSession->isLoggedIn() && !$this->appManager->isEnabledForUser($app, $this->userSession->getUser())) {
                                continue;
@@ -315,7 +316,8 @@ class NavigationManager implements INavigationManager {
                                }
                                $l = $this->l10nFac->get($app);
                                $id = $nav['id'] ?? $app . ($key === 0 ? '' : $key);
-                               $order = isset($nav['order']) ? $nav['order'] : 100;
+                               $order = (int)$this->config->getUserValue($user?->getUID(), 'core', 'apporder_'.$app.'_'.$key, '0');
+                               $order = $order ?: $nav['order'] ?? 100;
                                $type = $nav['type'];
                                $route = !empty($nav['route']) ? $this->urlGenerator->linkToRoute($nav['route']) : '';
                                $icon = isset($nav['icon']) ? $nav['icon'] : 'app.svg';

Note:
This is in navigationManager, more logical.
A configuration key is stored for each application navigation link entry, we may want to store an array for all of them instead.
For each navigation link we store an integer, I think this will behave nice when installing a new application it will go at its usual place, and the user can go in the settings to move it.

@come-nc
Copy link
Contributor

come-nc commented Sep 19, 2023

Note: defaultapp is already picked in core/defaultapp for each user, but I think there is no UI for it

@come-nc come-nc moved this from 📄 To do (~10 entries) to 🏗️ In progress in 📁 Files team Sep 19, 2023
@come-nc
Copy link
Contributor

come-nc commented Sep 25, 2023

On the format stored in the user configuration:
We need to store an integer for each app/key combination. We may use -1 as a special value later for hide.

Basically 3 possibilities:

  1. We store a config key for each app/key combination, like in the patch above, under 'apporder_'.$app.'_'.$key
    • Pros: easy to set through occ with occ user:setting admin core apporder_files_0 5 for instance
    • Cons: Lot of clutter in user config keys, harder to cleanup, leftovers on app uninstall
  2. We store an array under apporder
    • Pros: Cleaner in the config table
    • Cons: json encoding/decoding needed. Harder to set through occ.
  3. We store an array for each app under apporder_app
    • I think the worst solution because it has all the cons and non of the pros basically

I think all in all second option is the best one. If needed we can add occ command for this but it does not seem that important.

@sorbaugh
Copy link
Contributor Author

@come-nc I agree that nr.2 seems best for the current scope 👍

@susnux
Copy link
Contributor

susnux commented Oct 9, 2023

#40617

@susnux susnux closed this as completed Oct 9, 2023
@github-project-automation github-project-automation bot moved this from 🏗️ In progress to ☑️ Done in 📁 Files team Oct 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

No branches or pull requests

3 participants