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

Address issue -> Search memory in address bar #14 #63

Merged
merged 6 commits into from Mar 16, 2017
Merged

Address issue -> Search memory in address bar #14 #63

merged 6 commits into from Mar 16, 2017

Conversation

Rohanhacker
Copy link
Contributor

@Rohanhacker Rohanhacker commented Mar 12, 2017

screenshot from 2017-03-12 10-33-38

#63

Copy link
Collaborator

@Treora Treora left a comment

Choose a reason for hiding this comment

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

Great, looks neat. I made some comments in-line. One more thing: to keep things clean and modular, you could put all your code in a new file (call it omnibar, or omnibar-suggestions or something) in src/, and import that file from src/background.js

chrome.omnibox.onInputChanged.addListener(makeSuggestion)

// This event is fired with the user accepts the input in the omnibox.
chrome.omnibox.onInputEntered.addListener(acceptInput)
Copy link
Collaborator

Choose a reason for hiding this comment

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

chrome should be replaced with browser throughout the file. We pursue a cross-browser approach, and use webextension-polyfill to harmonise the APIs of Chrome|ium and perhaps others. Easiest is to follow MDN's docs while developing, it also contains info about browser compatibility.

import { filterVisitsByQuery } from './search'
import niceTime from './util/nice-time'

const makeSuggestion = (query, suggest) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps start with a setDefaultSuggestion to tell search is busy?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good idea, with that we can mitigate a slow search and people get feedback that something is happening.

if(searchResult.rows.length === 0) {
chrome.omnibox.setDefaultSuggestion({description: 'No results found'})
} else {
chrome.omnibox.setDefaultSuggestion({description: 'Select an option below'})
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds obvious. Is a default suggestion required, or could it just be removed when results arrive? Or perhaps something like "Found these pages in your memory:" would be helpful to explain where things came from.

let title = ''
let visitDate = niceTime(doc.visitStart)
let url = "<url>" + (content.slice(0,30)) + "</url>"
if(url.length > 30) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Testing the wrong variable. Did you mean content.length?

if(url.length > 30) {
url+= '...'
}
if(doc.page.extractedMetadata && doc.page.extractedMetadata.title) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You could use page.title, which is set here to whatever title is available. If no title is known at all (e.g. the page never loaded), it is set to the url though, which is a bit ugly; but in that case it will probably not show up in the search results either.

}
if(doc.page.extractedMetadata && doc.page.extractedMetadata.title) {
title = "<match>" + escape(doc.page.extractedMetadata.title) + "</match>"
title = title.replace(/%20|%2C|%3A|%3F|%B7|%23|%26/gi, matched => ' ')
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the idea here? When do titles contain these encoded characters?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

escape replaces -, #, spaces with %20, %2C etc so it replaces them with spaces otherwise It'll look ugly.

title = title.replace(/%20|%2C|%3A|%3F|%B7|%23|%26/gi, matched => ' ')
}
let description = `${url} ${(visitDate)} ${title}`
suggestions.push({
Copy link
Collaborator

Choose a reason for hiding this comment

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

You are performing a map, so no need to push elements: const suggestions = searchResult.rows.map(....)
Also, the function within map could be factored out and called visitToSuggestion or something understandable.

@Treora
Copy link
Collaborator

Treora commented Mar 16, 2017

@Rohanhacker: I did that bunch of edits myself as it seemed quicker than commenting on each aspect, I hope you agree with the changes. Unless you see problems still, I think this is ready to merge. The remaining to-do or other improvements can of course be resolved later.

The usual question for legal clarity: are you willing to waive all your copyrights on this code, to dedicate it to the public domain? (see e.g. unlicense.org)

@Rohanhacker
Copy link
Contributor Author

yup certainly.
I dedicate any and all copyright interest in this software to the
public domain. I make this dedication for the benefit of the public at
large and to the detriment of my heirs and successors. I intend this
dedication to be an overt act of relinquishment in perpetuity of all
present and future rights to this software under copyright law.

@Treora Treora merged commit d36f0a1 into WebMemex:master Mar 16, 2017
Treora added a commit that referenced this pull request Mar 16, 2017
@Treora
Copy link
Collaborator

Treora commented Mar 16, 2017

Merged. Thanks!

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.

None yet

3 participants