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

Added customize title bar options #212

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

UtiluMark
Copy link
Contributor

@UtiluMark UtiluMark commented Aug 23, 2016

  • Increased the size of the Customize Title bar window
  • Optimized the order of the Customize Title bar Variables
  • Added Customize Title bar Variable Application Title and Version
  • Added Customize Title bar Variable Application Display Version
  • Added Customize Title bar Variable Application Version Pretty
  • Added Customize Title bar Variable Application Update Channel
  • Added Customize Title bar Variable Application Update Channel Pretty (Release, ESR, beta, Aurora, Nighty, Default)
  • Added Customize Title bar Variable Application Beta Version (only filled when applicable), fixing Beta revision number not available for title #204
  • Added Customize Title bar Variable Application Version and Channel
  • Removed Gecko Build Identifier, since it used PlatformBuildID, just like XUL Platform Build Identifier
  • Removed Gecko Version, since it used PlatformVersion, just like XUL Platform Version

@whimboo
Copy link
Contributor

whimboo commented Aug 25, 2016

This PR will be one more fix for issue #188.

@UtiluMark, the commits in this PR look weird and I would like that you rebase against latest master. I see a lot of them which are not related to this PR at all. So please make sure that only the relevant ones are included in this PR, thanks.

@UtiluMark
Copy link
Contributor Author

@whimboo Rebased against latest master, so there is now only one commit.

@@ -2,6 +2,8 @@
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */

Components.utils.import("resource://gre/modules/AppConstants.jsm");
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will pollute global namespace.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved AppConstants import to where it is used: b568576
(AppConstants.MOZ_APP_VERSION_DISPLAY even works without the import)

Copy link
Collaborator

Choose a reason for hiding this comment

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

If it works without the import then I vote against importing it. :)

Copy link
Contributor Author

@UtiluMark UtiluMark Aug 28, 2016

Choose a reason for hiding this comment

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

Removed the AppConstants import completely: 9ae50b7

@xabolcs
Copy link
Collaborator

xabolcs commented Aug 28, 2016

May I ask to open a separate PR fixing #204 only?

@UtiluMark
Copy link
Contributor Author

@xabolcs The Beta Revision is only used in Customize Title bar Variables that were newly added in this PR, no current Customize Title bar Variables were changed. I don't think #204 should be fixed by changing current Customize Title bar Variables.

@xabolcs
Copy link
Collaborator

xabolcs commented Aug 28, 2016

@UtiluMark, would you mind thinking about to refactor this titlebar (and it's helpers) customization stuff into it's own jsm module?

It's OK to me if you leave it as is. But a nice refactor around titlebar customization is welcome!

@whimboo
Copy link
Contributor

whimboo commented Aug 29, 2016

I would also prefer a minimized patch to get support for the beta version number for issue #204, so that we can make it part of the 3.9 release. Thanks.

@UtiluMark
Copy link
Contributor Author

@whimboo Like I said #212 (comment) to @xabolcs, I don't think #204 can and should be fixed by changing current Customize Title bar Variables, since it's a missing feature and not a bug.
If you mean by "a minimized patch to get support for the beta version number" adding only the new Beta Revision Customize Title bar Variables from this PR through another PR, then that would still require updating the language files in that PR. Which means updating the language files twice, instead of once when done through this PR. So I think it's better to minimize the number of times the language files are updated, and just finish this PR.

* Increased the size of the Customize Title bar window
* Optimized the order of the Customize Title bar Variables
* Added Customize Title bar Variable Application Title and Version
* Added Customize Title bar Variable Application Display Version
* Added Customize Title bar Variable Application Version Pretty
* Added Customize Title bar Variable Application Update Channel
* Added Customize Title bar Variable Application Update Channel Pretty
(Release, ESR, beta, Aurora, Nighty, Default)
* Added Customize Title bar Variable Application Beta Revision (only
filled when applicable), fixing mozilla#204
* Added Customize Title bar Variable Application Version and Channel
* Removed Gecko Build Identifier, since it used PlatformBuildID, just
like XUL Platform Build Identifier
* Removed Gecko Version, since it used PlatformVersion, just like XUL
Platform Version
@UtiluMark
Copy link
Contributor Author

UtiluMark commented Aug 31, 2016

Note: If you want to test the code from this PR in a Firefox version that requires signed add-ons, you can use my signed fork.

@@ -15,20 +15,36 @@ variables: {
return this._appInfo;
},

get titleversion() { return this.tabtitle + ' - ' + this.vendor + ' ' + this.name + ' ' + this.versionchannel + ' (' + this.appbuildid + ')'; },
get defaulttitle() { return nightlyApp.defaultTitle; },
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better to review if you wouldn't have moved around existing lines. Another commit would have been good for that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also moving those lines would have made sense if those would be in alphabetical order now. But that is still not the case. Mind doing that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Title bar Variables are grouped per kind now.
I don't think an alphabetical order would provide a clear overview of the different variables with (partly) similar values (and if you would sort alphabetically, which row would you sort alphabetically? Variable? Description? Value?).

Copy link
Contributor

Choose a reason for hiding this comment

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

Common identifiers could be used first eg. titleDefault, titleTab. But whatever we use then other related variables like for tabs in this case are no longer grouped together. So whatever you do it will not be perfect.

@whimboo
Copy link
Contributor

whimboo commented Sep 1, 2016

As you can see there are lots of changes in this PR. I don't see that we can fold this into the 3.9 release. So lets move it to 3.10.

* Used the short way to get app.update.channel
* Changed beta revision to beta version
@UtiluMark
Copy link
Contributor Author

@whimboo @xabolcs @benplumley I've just released a new version with the latest changes from this PR, so if you want to test the latest code from this PR in a Firefox version that requires signed add-ons, you can use my signed fork.

Is there anything you would like to be added, removed or changed in this PR?

@benplumley
Copy link

@UtiluMark looks good, though I'm only able to test in release at the moment. I like the fine-grained variables, I'll likely use the signed fork until this is merged. Thanks!

@UtiluMark
Copy link
Contributor Author

@whimboo @xabolcs @benplumley Since this PR was moved to 3.10 #212 (comment), which is about to be released, while this PR isn't merged yet: can this PR be merged, or is there anything you would like to be added, removed or changed in this PR?

@whimboo
Copy link
Contributor

whimboo commented May 16, 2017

We have to get 3.10 out to unblock users from using it. It currently doesn't work for Nightly. Also I don't have the time to dive into this PR again for the moment. Sorry.

@UtiluMark
Copy link
Contributor Author

@whimboo @xabolcs @benplumley If you want to test the latest code from this PR with the latest changes from 3.10 in Nightly or a Firefox version that requires signed add-ons: I've just released a new version of my signed fork.

@xabolcs
Copy link
Collaborator

xabolcs commented Oct 11, 2017

@UtiluMark, may I close this PR or do you want to rebase it?

@UtiluMark
Copy link
Contributor Author

@xabolcs I was already working on it, but I had one issue, see #204 (comment).
We had found a way to retreive the beta version in the XUL-based extension, but this doesn't work in the WebExtension.
Do you know a way to retreive the beta version in the WebExtension?

@whimboo
Copy link
Contributor

whimboo commented Oct 11, 2017

Maybe bug 1386076 could help us here. It would give us the information on which channel the user is on. But so far it's not fixed.

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.

4 participants