Skip to content

Commit

Permalink
Issue mozilla#125 - Improve empty variable strings handling
Browse files Browse the repository at this point in the history
Leave "${Something}" untouched if nightly.variables doesn't have such property,
and let it be "Undefined" if it's a known variable, but currently unavailable.
For example in the "${Changeset}" case, when application.ini doesn't always
contain a "SourceStamp" key.
  • Loading branch information
xabolcs committed Apr 8, 2013
1 parent 3cf292d commit 8d81f70
Show file tree
Hide file tree
Showing 3 changed files with 11 additions and 5 deletions.
11 changes: 10 additions & 1 deletion extension/chrome/content/nightly.js
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,16 @@ getStoredItem: function(type, name) {
return nightly.preferences.getCharPref(type+"."+name);
}
catch (e) {}
return nightly[type][name];

if (nightly[type].hasOwnProperty(name)) {
varvalue = nightly[type][name];

This comment has been minimized.

Copy link
@whimboo

whimboo Apr 9, 2013

I assume you wanted to declare the value variable by using var, right? That way a varvalue variable will end up in the global scope.

This comment has been minimized.

Copy link
@xabolcs

xabolcs Apr 9, 2013

Author Owner

It's already defined locally near L140! :-)

This comment has been minimized.

Copy link
@whimboo

whimboo Apr 9, 2013

So please rename it to value to reduce confusions in the future.

if (varvalue === undefined || varvalue === null) {
varvalue = nightly.getString("nightly.variables.nullvalue");
}
return varvalue;
}

return undefined;
},

getVariable: function(name) {
Expand Down
4 changes: 0 additions & 4 deletions extension/chrome/content/titlebar/customize.js
Original file line number Diff line number Diff line change
Expand Up @@ -118,10 +118,6 @@ addVariable: function(name)
text="";
}
var value = paneTitle.nightly.getVariable(name);
if (value==null)
{
value="Undefined";
}
paneTitle.variables.push({variable: "${"+name+"}", description: text, value: value});
},

Expand Down
1 change: 1 addition & 0 deletions extension/chrome/locale/en-US/nightly.properties
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,4 @@ nightly.noextensions.message=No extensions were found.
nightly.restart.message=Addon compatibility has changed, restart to affect currently installed addons
nightly.restart.label=Restart
nightly.restart.accesskey=R
nightly.variables.nullvalue=Undefined

This comment has been minimized.

Copy link
@whimboo

whimboo Apr 9, 2013

Why don't you add this property to the variables.properties file? I think it fits better in there.

This comment has been minimized.

Copy link
@xabolcs

xabolcs Apr 9, 2013

Author Owner

Because I have concerns that nightly.getString could find strings not just from nightly.properties.
Btw I agree that it fits better in variables.properties.
Didn't tried.

This comment has been minimized.

Copy link
@xabolcs

xabolcs Apr 13, 2013

Author Owner

Decided to keep this new property in nightly.properties for now, but I happily move away in a refactor like mozilla#105 or mozilla#20.

This comment has been minimized.

Copy link
@whimboo

whimboo Apr 15, 2013

Fine by me if we don't make it fail in other applications.

0 comments on commit 8d81f70

Please sign in to comment.