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

Added the ability to see connectors that have been downloaded #1456

Closed
wants to merge 4 commits into from

Conversation

hypoflex
Copy link
Contributor

@hypoflex hypoflex commented Apr 5, 2020

With this addition you're able to see what connectors have already been downloaded,
since there's such great amount of connectors it can be daunting to know which one you used before. this feature is based on the reqeust from #535 and #1114

this code is far from optimal, best would be to convert Favorite.mjs into Connectors.mjs
but to proof it works i've done it this way. made it easier for me to play around without rewriting original code.

(07/04 13:00) Edit based on feedback from Ronny:

  • rewrite function to Storage.mjs and use already existing methods
  • Test on different platforms
  • Light theme version

(07/04 13:42) TODO:

  • Resolve the this.root variable to local variable using electron.remote.app.getPath('userData')
  • Combine register(file) and registerfav(file) to reduce duplicate code

@hypoflex hypoflex requested a review from ronny1982 April 5, 2020 15:00
src/app/ElectronBootstrap.js Outdated Show resolved Hide resolved
src/web/mjs/engine/Favorites.mjs Outdated Show resolved Hide resolved
@@ -30,6 +30,7 @@ export default class Storage {
this.fs = require( 'fs' );
this.path = require( 'path' );
this.config = this.path.join( electron.remote.app.getPath( 'userData' ), 'hakuneko.' );
this.root = electron.remote.app.getPath( 'userData');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

couldn't use this.config since it contains hakuneko in the path. i don't like rewriting original paths or duplicating variable values, this needs to change

Comment on lines +67 to +89
async registerfav(files) {
try {
for(let file of files) {
try {
let module = await import(file);
let connector = new module.default();
if(this._favlist.find(c => c.id === connector.id)) {
console.warn(`The connector "${connector.label}" with ID "${connector.id}" is already registered`);
} else {
this._favlist.push(connector);
}
} catch(error) {
console.warn(`Failed to load connector "${file}"`, error);
}
}
this._favlist.sort( ( a, b ) => {
return ( a.label.toLowerCase() < b.label.toLowerCase() ? -1 : 1 );
} );
} catch(error) {
console.warn(`Failed to load connector`, error);
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

only part I'm not happy with, since original register(files) uses this._list (which can't be rewritten) I've had to create a new register function.

Copy link
Contributor

@ronny1982 ronny1982 Apr 10, 2020

Choose a reason for hiding this comment

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

Method is not needed anymore if you accept the suggested change based on Storage.mjs

@hypoflex
Copy link
Contributor Author

hypoflex commented Apr 7, 2020

(07/04 13:42) TODO:

  • Resolve the this.root variable to local variable using electron.remote.app.getPath('userData')
  • Combine register(file) and registerfav(file) to reduce duplicate code

@@ -212,6 +212,34 @@
</template>
</div>
</div>
<div class="connectors">
Copy link
Contributor

@ronny1982 ronny1982 Apr 10, 2020

Choose a reason for hiding this comment

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

All this duplicated code should not be added.
Just introduce a new checkbox or something to toggle between the full list of websites and the websites that have been downloaded. In the code behind, assign the corresponding list to the property which is bound to the dom-repeat template in the UI.

onClick(evt) {
    this.connectorList  = evt.value ? /* downloaded website list */ this.favoriteList : /* full website list */;
}

@@ -212,6 +212,34 @@
</template>
</div>
</div>
<div class="connectors">
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as for the dark theme

@@ -911,4 +912,18 @@ export default class Storage {
} );
} );
}

get downloadedConnectors() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just return a list of connector IDs (string), makes further processing much simpler.

}

get list() {
return this._list;
}

get favlist() {
return this._favlist;
Copy link
Contributor

Choose a reason for hiding this comment

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

Just get the connector IDs of downloaded lists as suggested in Storage.mjs and then filter the full list based of the downloaded...
e.g.

let downloaded = Storage.getDownloadedConnectorIDs();
return this._list.filter(connector => downloaded.includes(connector.id));

@@ -24,16 +25,22 @@ export default class Connectors {
];
let userPlugins = await this._loadPlugins('hakuneko://plugins/');
let internalPlugins = await this._loadPlugins('hakuneko://cache/mjs/connectors/');
let favoritePlugins = Engine.Storage.downloadedConnectors;
Copy link
Contributor

@ronny1982 ronny1982 Apr 10, 2020

Choose a reason for hiding this comment

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

Not needed anymore if you accept the suggested change based on Storage.mjs


await this.register(systemPlugins);
await this.register(userPlugins);
await this.register(internalPlugins);
await this.registerfav(favoritePlugins);
Copy link
Contributor

@ronny1982 ronny1982 Apr 10, 2020

Choose a reason for hiding this comment

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

Not needed anymore if you accept the suggested change based on Storage.mjs

@@ -3,6 +3,7 @@ export default class Connectors {
constructor(ipc) {
ipc.listen('on-connector-protocol-handler', this._onConnectorProtocolHandler.bind(this));
this._list = [];
this._favlist = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

Not needed anymore if you accept the suggested change based on Storage.mjs

Copy link
Contributor

@ronny1982 ronny1982 left a comment

Choose a reason for hiding this comment

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

Looks good, but i would strongly recommend to take a look at the suggestions especially in the UI and Storage as these will greatly simplify the implementation and reuse existing concepts.

@hypoflex
Copy link
Contributor Author

Looks good, but i would strongly recommend to take a look at the suggestions especially in the UI and Storage as these will greatly simplify the implementation and reuse existing concepts.

again thanks for your feedback, going to pick this up asap

@ronny1982
Copy link
Contributor

Is this PR still active?

@ronny1982
Copy link
Contributor

Will close for now, maybe re-opened when work is continued

@ronny1982 ronny1982 closed this Sep 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants