-
-
Notifications
You must be signed in to change notification settings - Fork 813
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
Needed Interface for OSjs Search Engine #812
Comments
I had a look, and the This should probably be exposed via the |
Feel free to open a PR on this (or anyone stumbling across this). Should be a fairly easy modification :) |
Just thought of a thing. This could probably benefit from an Adapter pattern so that you don't have to override the actual // Changed core service
import VFSSearchAdapter from './adapters/search/vfs'
class Search {
constructor(core, options) {
// The options would come from the Service provider
const providedAdapters = options.adapters || []
const useAdapters = [VFSSearchAdapter, ...providedAdapters]
this.adapters = useAdapters.map(A => new A(core))
}
async init() {
await Promise.all(this.adapters.map(a => a.init()))
}
async search(pattern) {
const results = await Promise.all(this.adapters.map(a => a.search(pattern)))
// Maybe sorted ?! Maybe provide grouping in UI ?!
// Start simple with a flat result ?!
return results.flat(1)
}
} // Changed core service initialization
this.search = core.config('search.enabled')
? new Search(core, options.search || {})
: null; // A custom adapter in the distro
// Uses the standard interface as the rest of OS.js
export default class MySearchAdapter {
constructor(core) {
this.core = core
// If settings are needed it can be provided via a custom config entry
const config = core.config('search.mySearchAdapterSetttings', {})
this.settings = deepmerge({
someDefault: 'value',
}, config)
}
destroy() {}
async init() {
// For custom stuff during boot
}
async search(pattern) {
return []
}
} // Desktop service provider can now do this
import MySearchAdapter from './SearchAdapter'
osjs.register(DesktopServiceProvider, {
args: {
search: {
adapters: [MySearchAdapter],
}
}
}); |
Since we have previously developed and contributed our VFS Adapter, isn't it enough to only implement It seems here it loops over mountpoints and execute all search functions. |
Before going ahead, I create a PR for search function annotation. |
Yes. But adding this could allow for custom searches, not just VFS. It could return http links for example as paths and open tabs, etc :) |
Hi , Now in which file and where should we put this piece of code? |
This would be the `search.js` (core service) and `desktop.js` (where search
is inited) respectively.
And then move the existing search implementation to a new adapter (path in
import in example).
…On Mon, 5 Dec 2022, 19:33 mahdi norouzi, ***@***.***> wrote:
// Changed core service
import VFSSearchAdapter from './adapters/search/vfs'
class Search {
constructor(core, options) {
// The options would come from the Service provider
const providedAdapters = options.adapters || []
const useAdapters = [VFSSearchAdapter, ...providedAdapters]
this.adapters = useAdapters.map(A => new A(core))
}
async init() {
await Promise.all(this.adapters.map(a => a.init()))
}
async search(pattern) {
const results = await Promise.all(this.adapters.map(a => a.search(pattern)))
// Maybe sorted ?! Maybe provide grouping in UI ?!
// Start simple with a flat result ?!
return results.flat(1)
}
}
// Changed core service initialization
this.search = core.config('search.enabled')
? new Search(core, options.search || {})
: null;
Hi , Now in which file and where should we put this piece of code?
And what changes should we make?
—
Reply to this email directly, view it on GitHub
<#812 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABHODAQMXS2AW67YO2UZUTWLYYPPANCNFSM6AAAAAARMWS7IM>
.
You are receiving this because you commented.Message ID: <os-js/OS.
***@***.***>
|
Should a part of the search be deleted or how can I replace it? |
I can't go into much more detail because I'm on mobile.
The brief overview of code changes I gave should be enough to get started.
It's just a matter of moving some code and then implement an adapter
pattern + pass on some options from provider.
Open a PR as it's easier to do reviews than me just writing out the code
myself in issues.
…On Mon, 5 Dec 2022, 20:57 mahdi norouzi, ***@***.***> wrote:
@andersevenrud <https://github.com/andersevenrud>
Should a part of the search be deleted or how can I replace it?
—
Reply to this email directly, view it on GitHub
<#812 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABHODHAJE3BVJMPVPKLZWLWLZCJLANCNFSM6AAAAAARMWS7IM>
.
You are receiving this because you were mentioned.Message ID: <os-js/OS.
***@***.***>
|
@andersevenrud I am newcomer to this project
and file desktop.js in constructor:
|
Please open a PR so I can write reviews. It's a bit hard in issues.
…On Thu, 8 Dec 2022, 20:35 mahdi norouzi, ***@***.***> wrote:
@andersevenrud <https://github.com/andersevenrud>
hi
this file search.js :
Should I change like this?
*I am newcomer to this project*
export default class Search {
/**
* Create Search instance
* @param {Core} core Core reference
*/
constructor(core, options) {
const providedAdapters = options.adapters || []
const useAdapters = [VFSSearchAdapter, ...providedAdapters]
this.adapters = useAdapters.map(A => new A(core))
/**
* Core instance reference
* @type {Core}
* @readonly
*/
this.core = core;
/**
* Wired actions
* @type {Object}
*/
this.ui = null;
/**
* Last focused window
* @type {Window}
*/
this.focusLastWindow = null;
/**
* Search root DOM element
* @type {Element}
* @readonly
*/
this.$element = document.createElement('div');
}
init() {
const {icon} = this.core.make('osjs/theme');
const _ = this.core.make('osjs/locale').translate;
this.$element.className = 'osjs-search';
this.core.$root.appendChild(this.$element);
this.core.make('osjs/tray').create({
title: _('LBL_SEARCH_TOOLTOP', 'F3'),
icon: icon('system-search.png')
}, () => this.show());
this.ui = createUI(this.core, this.$element);
this.ui.on('hide', () => this.hide());
this.ui.on('open', iter => this.core.open(iter));
this.ui.on('search', query => {
this.search(query)
.then(results => this.ui.emit('success', results))
.catch(error => this.ui.emit('error', error));
});
await Promise.all(this.adapters.map(a => a.init()))
}
search(pattern) {
const results = await Promise.all(this.adapters.map(a => a.search(pattern)))
// Maybe sorted ?! Maybe provide grouping in UI ?!
// Start simple with a flat result ?!
const vfs = this.core.make('osjs/vfs');
const promises = this.core.make('osjs/fs')
.mountpoints()
.map(mount => `${mount.name}:/`)
.map(path => {
return vfs.search({path}, pattern)
.catch(error => {
logger.warn('Error while searching', error);
return [];
});
});
return results.flat(1)
}
....
....
and file desktop.js in constructor:
Should I change like this?
/**
* Search instance
* @type {Search|null}
* @readonly
*/
this.search = core.config('search.enabled') ? new Search(core) : null;
—
Reply to this email directly, view it on GitHub
<#812 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABHODAR53ICPQHH7R5WRDLWMI2AFANCNFSM6AAAAAARMWS7IM>
.
You are receiving this because you were mentioned.Message ID: <os-js/OS.
***@***.***>
|
I wanted to know that I did the change right? |
It's not fully correct.
If you open a PR i can make annotatilns where you need to fix stuff.
…On Thu, 8 Dec 2022, 20:39 mahdi norouzi, ***@***.***> wrote:
I wanted to know that I did the change right?
—
Reply to this email directly, view it on GitHub
<#812 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABHODBMX7PA6O4UVWWTQPLWMI2OHANCNFSM6AAAAAARMWS7IM>
.
You are receiving this because you were mentioned.Message ID: <os-js/OS.
***@***.***>
|
@andersevenrud |
@andersevenrud |
That was kind of the whole point of this. I wrote some ideas and suggestions on how it could be solved. If I were to point out (or write here) exactly what sections should be deleted/changed, I could just have written it myself. |
I wanted this to be an easy exercise into getting familiar with how everything works, and I thought that I had explained myself clearly. But maybe there is some kind of language barrier here or some knowledge gap which makes it very hard for me to give the information needed. |
Based on my comments, this is basically what's needed to make this work as I explained: New adapter file
|
@andersevenrud |
Excuse me, one question |
Hi.
How can we extend osjs-search-engine?
Imagine we have a search engine like
Elasticsearch
, what kind of interface osjs-search-engine needs to sync with our search engine?The text was updated successfully, but these errors were encountered: