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

Adding context menu to file manager according to packages installed on OSjs. #752

Closed
miladhashemi opened this issue Oct 4, 2020 · 10 comments · Fixed by os-js/osjs-client#143 or os-js/osjs-filemanager-application#31
Assignees
Labels

Comments

@miladhashemi
Copy link

Hi,
Some written application like Virus Scan do checking on files. I think it should be better to have dynamic context menu in filemanager. Consider bellow scenario:
We wrote a cloud based virus scan package that checks files. for this, we drag and drop file to virus scan. If there be a capability in filemanager that gets custom context menu from various packages, that could runs specific functionality, I think it could be awesome.
certainly it needs to change the way filemanager builds context menu and also packages must has option to register new context menu for filemanager.

@andersevenrud
Copy link
Member

I can probably add a way to define this in src/client/config.js as:

{
  filemanager: {
    contextmenu: [
      {
        type: 'file' | 'dir' | 'special',
        event: 'myEventName',
        label: 'Something' // Automatically translated if defined in either filemanger locales or core cocales
      }
    ]
  }
}

You can then register the following event anywhere:

core.on('osjs/filemanager:contextmenu:myEventName', (file) => {
  // Do what you want here
})

Does this sound good ?

@andersevenrud
Copy link
Member

andersevenrud commented Oct 4, 2020

custom context menu from various packages

Since packages are loaded dynamically there's no way to have a package do something in another package without launching both first.

So it would have to be something like I mentioned in my previous comment. However, it's possible to include a configuration bootstrap in your packages to make it sort of possible, but statically.

Example:

// my-package/config.js
export function register(core) {
  core.on('osjs/filemanager:contextmenu:myEventName', (file) => {
    // Do what you want here
    // Like launch the app you want with this file, or send the file to an already open app
  })
}

export const config = {
  filemanager: {
    contextmenu: []
  }
}
// OS.js/src/client/config.js
import { config as myPackageConfig } from 'my-package/config.js'
import { config as someOtherPackageConfig } from 'other-package/config.js'
import deepmerge from 'deepmerge'

const config = deepmerge({
  // ... standard config here...
}, myPackageConfig, someOtherPackageConfig)

export default config
// OS.js/src/client/index.js
import { register as registerMyPackage } from 'my-package/config.js'

const init = () => {
  // ...
  registerMyPackage(osjs)
  // ...
}

@andersevenrud
Copy link
Member

Come to think of it, it would be possible to add the actual configuration section into metadata.json of a package, but the actual event has to be registered at some point in order for it to take effect.

@msjavan
Copy link

msjavan commented Oct 5, 2020

I can probably add a way to define this in src/client/config.js as:

{
  filemanager: {
    contextmenu: [
      {
        type: 'file' | 'dir' | 'special',
        event: 'myEventName',
        label: 'Something' // Automatically translated if defined in either filemanger locales or core cocales
      }
    ]
  }
}

You can then register the following event anywhere:

core.on('osjs/filemanager:contextmenu:myEventName', (file) => {
  // Do what you want here
})

Does this sound good ?

I think what you mentioned here could be a good solution. Having a registry for the file-manager context menu, a third party application (e.g. virus scan, ...) can register itself.

@ThisIsOstad
Copy link

Since packages are loaded dynamically there's no way to have a package do something in another package without launching both first.

Hi, Thank you for your awesome work.

I'm getting started working on OS.js and my comment may not be valid.

IMO it's better to keep packages portable and keep all of their configs in the package. Also, it may be useful to be able to customize context-menu items dynamically. For example, think of an application like WinRAR. It needs to add an item in the context-menu (say "Extract") only for compressed files.

Is there a way for FileManager to keep its context-menu items somewhere in the core so that every other package can customize it dynamically?

// my-package/config.js
export function register(core) {
  core.on('osjs/filemanager:contextmenu:beforerender', (file, contextMenuItems) => {
    // Do what you want here
    // Like adding/removing items to/from contextMenuItems and pass it to the next
    return contextMenuItems
  })
}

This will register the passed function in the core where context-menu items are living. When FileManager wants to render the context-menu, it needs to read it from the thing in the core, and it should run all of the registered functions to calculate the final contextMenuItems. Does it make any sense?

@miladhashemi
Copy link
Author

Does this sound good ?

It sounds good. But even could be seen more general, not only for filemanager package but also for every packages. for example a spread sheet package could has context menu to translate sentences.

So it would have to be something like I mentioned in my previous comment. However, it's possible to include a configuration bootstrap in your packages to make it sort of possible, but statically.

I think it has an advantage if a package developer doesn't need to have registers packages to src/client/config.js manualy, it is clear there is requirement to have dynamic config discovery mechanism to register each package config automatically. I think by this way a developer just needs to know how develops a package and make his own config.

@andersevenrud
Copy link
Member

andersevenrud commented Oct 5, 2020

This will register the passed function in the core where context-menu items are living. When FileManager wants to render the context-menu, it needs to read it from the thing in the core, and it should run all of the registered functions to calculate the final contextMenuItems. Does it make any sense?

Makes sense :)

it is clear there is requirement to have dynamic config discovery mechanism to register each package config automatically. I think by this way a developer just needs to know how develops a package and make his own config.

I like @MoradiDavijani's idea because it removes the need for any configuration manipulation (and therefore no merging required).

So here's a brain dump...

Doing that via discovery or dynamic loading has a couple of disadvantages:

  1. No longer a part of the core bundle, which makes for a bit more overhead in overall loading size
  2. Would have to be loaded upon startup, which increases loading time. And since it's not part of the core bundle there's 1 file per package that would have to be loaded.
    • Unless if it were possible to do this from the standard "main.js" file where you register the application. But this would require loading the package bundle upon startup. Which takes longer to load than a separate file just for the contextmenu registration.

But also comes with some advantages:

  1. No more configuration manipulation
  2. Does not require npm run build to take effect

Maybe the ease of use here outweighs the disadvantages ?! I'm usually a bit concerned about loading extra assets upon startup, but if this was split up into a separate file it would not be that much of an impact, especially seeing that this would be mostly used for third party stuff. It could also be loaded in the background.


But even could be seen more general

Yeah, this is probably a good idea. And maybe make not even just for context menu stuff, but arbitrary things. I'm thinking something like this:

// Service contract
const middlewareContract = {
  add: (group, cb) => {},
  get: (group) => {}
}

core.singleton('osjs/middleware', middlewareContract)

// Global API exposure
window.OSjs = {
  // ...
  middleware: (group, cb) => middlewareContract.add(group, cb)
}
// Package metadata
{
  // This would have to be added to the webpack config as well
  "preload": [
    {
      "background": true,
      "filename": "middleware.js"
    }
  ]
}
// Package middleware definition "middleware.js"
import osjs from 'osjs'

osjs.middleware('osjs/filemanager:contextmenu:edit', (({ file }) => {
  if (file.isDir) {
    return []
  }

  return [
    {
      label: 'Custom entry',
      onclick: () => {}
    }
  ]
})
// Filemanager implementation
const createAdditionalEditContextMenu = async (core, file) => {
  const middleware = core.make('osjs/middleware').get('osjs/filemanager:contextmenu:edit')
  const menus = asyncMap(middleware, m => m({ file }))
  return [].concat(...menus)
}

@andersevenrud
Copy link
Member

I'm going to prematurely close this issue even though it was linked in a PR.

This has been super-seeded by os-js/osjs-client#144

@andersevenrud
Copy link
Member

This has been released with @osjs/client@3.5.0 and @osjs/filemanager-application@1.5.0 🎉

@miladhashemi
Copy link
Author

Thank you guys so much 🥇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants