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.

Sign up for GitHub

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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions _locales/en/messages.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,9 @@
"seeCustomizationTips_url": {
"message": "http://brief.mozdev.org/customize.html"
},
"openInPopup_label": {
"message": "Open Brief in a popup"
},
"feedName_label": {
"message": "Name"
},
Expand Down
3 changes: 3 additions & 0 deletions background.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ const Brief = {
await Prefs.init({master: true});

Prefs.addObserver('showUnreadCounter', () => this._updateUI());
Prefs.addObserver('openInPopup', () => this._updateUI());
Comm.registerObservers({
'feedlist-updated': () => this._updateUI(),
'entries-updated': debounced(100, () => this._updateUI()),
Expand Down Expand Up @@ -191,6 +192,7 @@ const Brief = {
_updateUI: async function() {

let enabled = Prefs.get('showUnreadCounter');
let isPopup = Prefs.get('openInPopup');
browser.contextMenus.update('brief-button-show-unread', {checked: enabled});
if(enabled) {
let count = await Database.query({
Expand All @@ -209,6 +211,7 @@ const Brief = {
} else {
browser.browserAction.setBadgeText({text: ""});
}
browser.browserAction.setPopup({popup: isPopup ? '/ui/brief.xhtml' : ''});
//TODO: return tooltip
/*
_updateStatus: async function Brief__updateStatus() {
Expand Down
2 changes: 1 addition & 1 deletion manifest.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
"commands": {
"_execute_browser_action": {
"suggested_key": {
"default": "Ctrl+Shift+D"
"default": "Ctrl+Shift+F"
Copy link
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
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.

}
}
},
Expand Down
1 change: 1 addition & 0 deletions scripts/prefs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ function pref(name, value) {
// The actual prefs
pref("homeFolder", -1);
pref("showUnreadCounter", true);
pref("openInPopup", false);
pref("firstRun", true);
pref("lastVersion", "0");
pref("assumeStandardKeys", true);
Expand Down
2 changes: 1 addition & 1 deletion scripts/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ export function getPluralForm(number, forms) {


export async function openBackgroundTab(url) {
let tab = await browser.tabs.getCurrent();
let [tab] = await browser.tabs.query({active: true, currentWindow: true});
Copy link
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?

Copy link
Author

@katzelad katzelad May 10, 2019 •
edited
Loading

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.

try {
await browser.tabs.create({active: false, url: url, openerTabId: tab.id});
}
Expand Down
1 change: 1 addition & 0 deletions skin/brief.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ body.sidebar #reveal-sidebar-button {
#sidebar {
background-color: #f2f2f2 !important;
min-width: 250px !important;
min-height: 600px !important;
}

#sidebar-splitter,
Expand Down
4 changes: 4 additions & 0 deletions ui/options/options.xhtml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,10 @@
</div>
<div>
<h2 data-i18n="editStyleDescription_label"/>
<label data-i18n="openInPopup_label">
<input type="checkbox" data-pref="openInPopup"/>
</label>
<br />
<textarea id="custom-style-textbox"/>
<a id="learn-more-link"
data-i18n="seeCustomizationTips_label"
Expand Down