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

add support for most search engines #31

Merged
merged 13 commits into from Dec 9, 2022
Merged

Conversation

josStorer
Copy link
Contributor

@josStorer josStorer commented Dec 6, 2022

related: #1 #8 #20 #21 #22 #24

Now support Google, Bing, Yahoo, DuckDuckGo, StartPage, Baidu, Kagi, Yandex, Naver, Brave, Searx, Ecosia
Have a try here: https://github.com/josStorer/chat-gpt-search-engine-extension/releases

Preview:

bing
duckduckgo
google
kagi
naver
startpage
yahoo jp
yahoo
yandex
baidu
brave
searx
ecosia

@josStorer
Copy link
Contributor Author

josStorer commented Dec 6, 2022

related: #1 #8 #20 #21 #22 #24

@josStorer josStorer mentioned this pull request Dec 6, 2022
@wong2
Copy link
Owner

wong2 commented Dec 6, 2022

Wow thanks, I'll take a look tomorrow

import Browser from 'webextension-polyfill'
import {getPossibleElementByClassArray, getPossibleElementByIdArray, getPossibleElementByNameArray} from './utils.mjs'

const config = {

Choose a reason for hiding this comment

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

Instead of doing it this way which will get messy and harder to maintain. Having a configuration for each search engine then it will be a single lookup instead doing it for all arrays. Easier to maintain if we split this up per search engine.

Copy link
Contributor Author

@josStorer josStorer Dec 7, 2022

Choose a reason for hiding this comment

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

Instead of doing it this way which will get messy and harder to maintain. Having a configuration for each search engine then it will be a single lookup instead doing it for all arrays. Easier to maintain if we split this up per search engine.

agree with you, this is a 'just work' version

Choose a reason for hiding this comment

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

Much better!

@Plutone11011 Plutone11011 mentioned this pull request Dec 6, 2022
@kxxt
Copy link

kxxt commented Dec 7, 2022

This is a review generated by ChatGPT 🤩:

This pull request appears to add support for multiple search engines to a browser extension that displays ChatGPT responses alongside search results. The extension currently only supports Google, but this pull request aims to add support for Bing, Yahoo, DuckDuckGo, StartPage, Baidu, Kagi, Yandex, Naver, Brave, Searx, and Ecosia.

One potential problem with this pull request is that it hardcodes the input element names for search boxes into the config variable. This means that if any of the search engines supported by this extension change the name of their search box input elements, the extension will no longer be able to find them and will not be able to display ChatGPT responses alongside search results.

To fix this issue, I would suggest using a more flexible approach for identifying search box input elements. One way to do this would be to use the querySelectorAll method to search for elements with the input tag and the search or query attribute, rather than hardcoding specific input element names into the config variable.

Another potential problem with this pull request is that it adds a large number of new configuration options to the config variable, which makes the code difficult to read and understand. To improve readability and make it easier to maintain the code, I would suggest moving the configuration options for each search engine into a separate object. This would make it easier to add, remove, or modify the configuration options for individual search engines without having to scroll through a long list of options.

Overall, I think this pull request has the potential to add useful functionality to the extension, but it would benefit from some changes to improve its flexibility and readability.

@wong2
Copy link
Owner

wong2 commented Dec 7, 2022

@kxxt That's cool

@josStorer
Copy link
Contributor Author

@wong2 are you currently working on this PR? If not, I can do some updates tomorrow to make it more flexible and readable

@wong2
Copy link
Owner

wong2 commented Dec 7, 2022

@josStorer I'm not.

Copy link

@mohamedmansour mohamedmansour left a comment

Choose a reason for hiding this comment

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

Much better, now hope search engines don't change layouts or selector names :-)

@wong2
Copy link
Owner

wong2 commented Dec 8, 2022

Please resolve conflicts

@josStorer
Copy link
Contributor Author

@wong2 resolved

@jonathansampson
Copy link

Nice work, @josStorer!

}
);
return element;
}
Copy link
Owner

Choose a reason for hiding this comment

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

I'd prefer using simple for...of loop for these functions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry I don't know what happened when I wrote this code, it does seem like a long and incomprehensible way 😂

const siteNameReplaceList = [document.location.hostname, "www.", "search.", "cn."]
const siteName = siteNameReplaceList.reduce((pre, cur) => {
return pre.replace(cur, "")
}).match(/(.+?)\./)[1]
Copy link
Owner

Choose a reason for hiding this comment

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

This is hard to read and understand

Copy link
Contributor Author

Choose a reason for hiding this comment

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

replace the prefix of hostname, and then get first word before the dot with regex

do you think it would be better to change the implement, or just add some comments

Copy link
Owner

@wong2 wong2 Dec 8, 2022

Choose a reason for hiding this comment

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

I think we can create a regex with Object.keys(config) and use it to match location.hostname

Copy link
Contributor Author

Choose a reason for hiding this comment

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

get it!

"https://*/search*",
"https://duckduckgo.com/*q=*",
"https://*.startpage.com/sp/search*",
"https://*.baidu.com/s*wd=*"
Copy link
Owner

Choose a reason for hiding this comment

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

I think we can change this to https://*/*?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, we can

@josStorer
Copy link
Contributor Author

josStorer commented Dec 8, 2022

@wong2 have updated and tested, it works fine

sidebarContainerId: ["rhs"],
sidebarContainerClass: [],
appendContainerId: ["rcnt"],
appendContainerClass: []
Copy link
Owner

Choose a reason for hiding this comment

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

maybe we can combine id and class, by using querySelector

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, wait a minute

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wong2 finished

@wong2
Copy link
Owner

wong2 commented Dec 8, 2022

thanks, I'll merge after more tests tomorrow

@wong2
Copy link
Owner

wong2 commented Dec 9, 2022

I'll merge this and do some fixes myself.

@wong2 wong2 merged commit 24f843f into wong2:main Dec 9, 2022
@ghost
Copy link

ghost commented Dec 21, 2022

大佬厉害

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

Successfully merging this pull request may close these issues.

None yet

5 participants