Skip to content

Added option to open Brief in a popup instead of a new tab - #448

Open
katzelad wants to merge 1 commit into
brief-rss:masterfrom
katzelad:devel
Open

Added option to open Brief in a popup instead of a new tab#448
katzelad wants to merge 1 commit into
brief-rss:masterfrom
katzelad:devel

Conversation

@katzelad

@katzelad katzelad commented May 7, 2019

Copy link
Copy Markdown

Added option to open Brief in a popup instead of a new tab (#328 #339 #433)
Modified global shortcut key to ctrl+shift+f (since ctrl+shift+d does not work)

Modified global shortcut key to ctrl+shift+f (since ctrl+shift+d does not work)
Comment thread manifest.json
"_execute_browser_action": {
"suggested_key": {
"default": "Ctrl+Shift+D"
"default": "Ctrl+Shift+F"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No idea why it worked when Brief was first migrated to WebExtensions. I'd prefer to switch back to Ctrl+Alt+D if this does not break Firefox 60ESR compatibility.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The "Bookmark All Tabs" shortcut used to be disabled on Linux before Firefox 65 (bug 1504205), thus I've missed its existence.

Comment thread scripts/utils.js

export async function openBackgroundTab(url) {
let tab = await browser.tabs.getCurrent();
let [tab] = await browser.tabs.query({active: true, currentWindow: true});

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This seems to be not a good idea. It'll cause the new tab to be opened as if the currently active tab opened it, but the currently active tab is just the tab you happened to be on when you peeked into Brief... does it really make sense to bind the new tab to it?

@katzelad katzelad May 10, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@tanriol I figured this was a bug, since it seems the original intention was to achieve the same goal using browser.tabs.getCurrent() (which does not return the current tab when called from a background script).
This fix makes it so that when Brief is closed the last tab you were on is displayed, which is usually the tab I want (returning to the workflow after a quick peek). This is also the purpose of the popup feature.

@tanriol

tanriol commented May 10, 2019

Copy link
Copy Markdown
Member

This is an interesting idea, but the Brief UI as done today does not feel a good match for a popup. In my opinion, it would be an interesting idea to have such a mode, but I'm not sure about making it a feature yet.

I'd be fine with merging the support as a hidden option (pref-only, needs to be enabled from console) for now.

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