Fixes #1484 Create pageName type #1813
Conversation
re-add readability spaces
Simpcyclassy
force-pushed
the
pageName-type
branch
from
37da7e6 to
6d68ba4
Compare
extension/background/entityTypes.js
Outdated
| for (const id in metadata.pageNames) { | ||
| const item = metadata.pageNames[id]; | ||
| for (const name of item.names) { | ||
| namedPages.push(name); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
I understand I need to create a new Alternative here can I get a bit more clarity on that @ianb
| return pageNames; | ||
| } | ||
|
|
||
| export async function registerPageName(name, { url }) { |
There was a problem hiding this comment.
I assume this is the function where I am to save the pageNames to the entityTypes, it's not clear to me how the services communicate
There was a problem hiding this comment.
Yes, this should call a new function in entityTypes.js
| name: namedPages, | ||
| lang: languageNames(), | ||
| smallNumber: English.numberNames, | ||
| }); |
There was a problem hiding this comment.
convertEntities does its own thing. It might actually be OK, but then if you look at entityTypes.name (it would be better to call it .pageName) you'll see it is an instance of Alternatives.
A good approach here is to use things like console.log(entityTypes.name) to see what's what.
But to add a new thing to the entity you'd do something like:
export function addPageName(name) {
entityTypes.pageNames.alternatives.push(makeWordMatcher(name));
}(Note makeWordMatcher isn't exported at the moment, but could be)
| return pageNames; | ||
| } | ||
|
|
||
| export async function registerPageName(name, { url }) { |
There was a problem hiding this comment.
Yes, this should call a new function in entityTypes.js
| const query = context.slots.query.toLowerCase(); | ||
| const result = await browser.storage.sync.get("pageNames"); | ||
| const pageNames = result.pageNames; | ||
| const name = context.slots.name.toLowerCase(); |
There was a problem hiding this comment.
I don't think these changes to navigation.js are necessary or related to the rest of the changes
extension/background/intentRunner.js
Outdated
| const creationDate = Date.now(); | ||
| const result = await browser.storage.sync.get("pageNames"); | ||
| pageNames = result.pageNames || {}; | ||
| pageNames = getRegisteredPageName() || {}; |
There was a problem hiding this comment.
We probably won't need this any longer. Instead we can create a new intent that just matches the phrase:
[name:pageName]
And the new intent should look up and open the page.
extension/background/intentRunner.js
Outdated
| const creationDate = Date.now(); | ||
| const result = await browser.storage.sync.get("pageNames"); | ||
| pageNames = result.pageNames || {}; | ||
| const pageNames = getRegisteredPageName() || {}; |
| async run(context) { | ||
| const name = context.slots.name; | ||
| await intentRunner.getRegisteredPageName(name); | ||
| const pageNames = await intentRunner.getRegisteredPageName(name); |
There was a problem hiding this comment.
Here getRegisteredPageName() is used with an argument, but the function as defined doesn't take any arguments.
Really there's a confusion here, as getRegisteredPageName() is also used elsewhere without an argument (in registerPageName()). But in that case it's being used incorrectly.
Be sure when you are changing a function that you look for everywhere where the function is used, to make sure you are changing it in a way that's compatible with how it's being used.
andrenatal
force-pushed
the
master
branch
3 times, most recently
from
7ad02c2 to
0ec2a05
Compare
2 participants
No description provided.