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

UX Improvements #25

Merged
merged 1 commit into from
Aug 2, 2023
Merged

UX Improvements #25

merged 1 commit into from
Aug 2, 2023

Conversation

BrunoBernardino
Copy link
Contributor

@BrunoBernardino BrunoBernardino commented Aug 2, 2023

This includes a lot of UX improvements and cleanup, done while the research for other features/improvements was underway:

Additionally, I looked into sending the tab's HTML/Text content instead of the URL with a setting/dropdown (https://discord.com/channels/849884108750061568/1050508822864732170/1125156451015475364), but decided against it, because we would need access to the tab's DOM content, and the only ways I found to do that were too invasive in terms of permissions (scripting, which could be optional, and wildcard host, which could not be optional). Source

Fixes #19
Fixes #3
Closes #24


kagi_chrome_0.3.6.zip

kagi_firefox_0.3.6.zip


attempt-to-add-text

screen-before-permissions

new-free-indication

This includes a lot of UX improvements and cleanup, done while the research for other features/improvements was underway:

- The extension can now be used without granting the `activeTab` permission, where it'll just behave as it did before `0.3`, simply allowing Kagi to be used in private sessions (#19)
- The extension should now be fully usable in Firefox for Android (#3)
- The extension will now look for the active tab of the last focused window (alternative fix to Edge issue reported in #24 with multiple windows)
- I confirmed the extension works as expected in Edge (and Brave, besides Chrome) without any new (invasive) permissions (#23)
- Using Cecil will now not call the API, even if an API Key is set (https://kagifeedback.org/d/1751-enable-kagi-api-users-to-use-universal-summarizer-cecil-without-incurring-api-charges)
- Confirmed there's no other current way of storing the session that survives clearing of session data (https://kagifeedback.org/d/1754-restarting-the-firefox-browser-signs-me-out-of-kagi)

Additionally, I looked into sending the tab's HTML/Text content instead of the URL with a setting/dropdown (https://discord.com/channels/849884108750061568/1050508822864732170/1125156451015475364), but decided against it, because we would need access to the tab's DOM content, and the only ways I found to do that were too invasive in terms of permissions (`scripting`, which could be optional, and wildcard host, which could not be optional). [Source](https://stackoverflow.com/questions/19758028/chrome-extension-get-dom-content)

Fixes #19
Fixes #3
Closes #24
@BrunoBernardino BrunoBernardino self-assigned this Aug 2, 2023
@@ -47,7 +47,8 @@
},
"browser_specific_settings": {
"gecko": {
"id": "[email protected]"
"id": "[email protected]",
"strict_min_version": "102.0"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was the main thing preventing the extension from being compatible with Firefox. This version (102) is 1 year old, so I don't expect people to have problems not being able to install the extension.

Comment on lines +16 to +18
const useApi = Boolean(
api_token && ((api_engine && api_engine !== 'cecil') || text),
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now we'll only use the API if there's a an API Key and the engine isn't Cecil.

@@ -95,19 +97,41 @@ export async function summarizeContent({
};
}

export async function updateSettings(handleGetData) {
export async function fetchSettings() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made this function more "standard" (not taking a callback, returning the object).

tab?.url?.startsWith('http://') || tab?.url?.startsWith('https://'),
) || tabs[0];

if (!tab || !tab.url) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Found this new case, after permissions were granted, before they were requested, where we'd get a tab without a url. That would cause an "Invalid URL" error. The "fix" was simply to close the extension after requesting the permissions for the first time (so we can load it up with permissions).

@@ -77,113 +78,135 @@ function setStatus(type) {
}

async function setup() {
const eTokenDiv = document.querySelector('#token');
Copy link
Contributor Author

@BrunoBernardino BrunoBernardino Aug 2, 2023

Choose a reason for hiding this comment

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

I also renamed a lot of variables and functions as I was going through some other cleanup of logic and adding a new option (which was then removed, for the URL/text select).

Comment on lines +229 to +231
const hasPermissions = await browser.permissions.contains({
permissions: ['activeTab'],
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

One interesting, hard to debug fact is that Firefox will complain if you try to check for permissions and request them on the same event loop. That's why I ended up having to create a separate button/section (initially I just had the selects hide and the summarize button change its text). Chrome didn't care, though.

Comment on lines +57 to +60
summaryTextContents = new DOMParser().parseFromString(
data.summary.replaceAll(/<br>/g, '\n'),
'text/html',
).documentElement.textContent;
Copy link
Contributor Author

@BrunoBernardino BrunoBernardino Aug 2, 2023

Choose a reason for hiding this comment

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

This was necessary because of some escaped HTML entities we receive from the summarizer, and because we're not setting it as text, not HTML.

@@ -63,50 +66,50 @@ async function setup() {
}

summaryResultElement.style.display = '';
summaryResultElement.innerHTML = data.summary.replaceAll(/\n/g, '<br />');
summaryResultElement.innerText = summaryTextContents;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was the other piece required to be changed for the Firefox for Android extension to not show a warning.

Comment on lines +74 to +83
const hasTabAccess = await browser.permissions.contains({
permissions: ['activeTab'],
});

if (!hasTabAccess) {
summaryResultElement.style.display = '';
summaryResultElement.classList.add('error');
summaryResultElement.innerText =
"You can't summarize without allowing access to the currently active tab.";
return;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just a "safety check" in case the shortcut is set and called before permissions are given (can't request them from here easily since it needs a button click, as an example -- it seemed unnecessary to rebuild that UI here, for this case, as it already exists in the main popup).

@vprelovac vprelovac merged commit 5d42c3b into main Aug 2, 2023
@BrunoBernardino BrunoBernardino deleted the feature/ux-improvements branch August 2, 2023 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants