launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04532
[Merge] lp:~allenap/launchpad/packageset-picker-2 into lp:launchpad
Gavin Panella has proposed merging lp:~allenap/launchpad/packageset-picker-2 into lp:launchpad with lp:~allenap/launchpad/packageset-picker as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #814531 in Launchpad itself: "PackagesetPickerWidget does not consistently fire choicesChange events"
https://bugs.launchpad.net/launchpad/+bug/814531
For more details, see:
https://code.launchpad.net/~allenap/launchpad/packageset-picker-2/+merge/70915
This branch change anything that depends on or inherits from
SelectWidget to use ChoiceListWidget, and also adjusts those widgets
that inherit from ChoiceListWidget and those places where
ChoiceListWidget is used directly to cope with its new behaviour (see
prerequisite branch).
--
https://code.launchpad.net/~allenap/launchpad/packageset-picker-2/+merge/70915
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/launchpad/packageset-picker-2 into lp:launchpad.
=== modified file 'lib/lp/app/javascript/formwidgets/tests/test_formwidgets.js'
--- lib/lp/app/javascript/formwidgets/tests/test_formwidgets.js 2011-08-09 16:26:09 +0000
+++ lib/lp/app/javascript/formwidgets/tests/test_formwidgets.js 2011-08-09 16:26:11 +0000
@@ -434,6 +434,34 @@
"a", this.widget.get("choice").value);
},
+ testAddChoices: function() {
+ this.widget.add_choices(
+ [{value: 'c', text: 'c', data: 'c'}]);
+ ArrayAssert.itemsAreEqual(
+ ["c"],
+ attrselect("text")(this.widget.get("choices")));
+ this.widget.add_choices(
+ [{value: 'a', text: 'a', data: 'a'}]);
+ ArrayAssert.itemsAreEqual(
+ ["a", "c"],
+ attrselect("text")(this.widget.get("choices")));
+ this.widget.add_choices(
+ [{value: 'b', text: 'b', data: 'b'}]);
+ ArrayAssert.itemsAreEqual(
+ ["a", "b", "c"],
+ attrselect("text")(this.widget.get("choices")));
+ /* Adding a new choice that has the same value as an
+ existing choice overwrites the existing choice. */
+ this.widget.add_choices(
+ [{value: 'b', text: 'z', data: 'z'}]);
+ ArrayAssert.itemsAreEqual(
+ ["a", "c", "z"],
+ attrselect("text")(this.widget.get("choices")));
+ ArrayAssert.itemsAreEqual(
+ ["a", "c", "z"],
+ attrselect("data")(this.widget.get("choices")));
+ },
+
testAddChoicesEvents: function() {
/* Calling add_choices() causes choicesChange and
choiceChange events to fire. */
=== modified file 'lib/lp/registry/javascript/distroseries/initseries.js'
--- lib/lp/registry/javascript/distroseries/initseries.js 2011-07-18 10:50:01 +0000
+++ lib/lp/registry/javascript/distroseries/initseries.js 2011-08-09 16:26:11 +0000
@@ -14,8 +14,9 @@
var namespace = Y.namespace('lp.registry.distroseries.initseries');
-var widgets = Y.lp.registry.distroseries.widgets;
-var formwidgets = Y.lp.app.formwidgets;
+var widgets = Y.lp.registry.distroseries.widgets,
+ formwidgets = Y.lp.app.formwidgets,
+ attrselect = Y.lp.extras.attrselect;
/**
@@ -75,6 +76,7 @@
*/
submit: function() {
var self = this;
+ var values = attrselect("value");
var config = {
on: {
start: function() {
@@ -89,11 +91,12 @@
name: this.context.name,
distribution: this.context.distribution_link,
parents: this.deriveFromChoices.get("parents"),
- architectures: this.architectureChoice.get("choice"),
+ architectures:
+ values(this.architectureChoice.get("choice")),
packagesets: this.packagesetChoice !== null ?
- this.packagesetChoice.get("choice") : [],
- rebuild: this.packageCopyOptions.get("choice") === (
- "Copy Source and Rebuild"),
+ values(this.packagesetChoice.get("choice")) : [],
+ rebuild:
+ this.packageCopyOptions.get("choice").value === "rebuild",
overlays: this.deriveFromChoices.get("overlays"),
overlay_pockets: this.deriveFromChoices.get(
"overlay_pockets"),
@@ -209,13 +212,14 @@
var package_copy_options =
new formwidgets.ChoiceListWidget()
.set("name", "field.package_copy_options")
- .set("type", "radio")
+ .set("multiple", false)
.set("label", "Copy options:")
.set("description", (
"Choose whether to rebuild all the sources you copy " +
"from the parent(s), or to copy their binaries too."))
- .set("choices", ["Copy Source and Rebuild",
- "Copy Source and Binaries"])
+ .set("choices", [
+ {text: "Copy Source and Rebuild", value: "rebuild"},
+ {text: "Copy Source and Binaries", value: "copy"}])
.set("choice", "Copy Source and Binaries")
.render(form_table_body);
var form_actions =
@@ -273,7 +277,7 @@
else {
// Disable rebuilding if cache.is_first_derivation is false.
form_actions.packageCopyOptions.fieldNode
- .one('input[value="Copy Source and Rebuild"]')
+ .one('input[value="rebuild"]')
.set('disabled', 'disabled');
form_actions.packageCopyOptions.set("description", (
"Note that you cannot rebuild sources because the " +
@@ -304,6 +308,7 @@
};
-}, "0.1", {"requires": ["node", "dom", "io", "widget", "array-extras",
- "transition", "lp.registry.distroseries.widgets",
- "lp.app.formwidgets", "lp.app.picker"]});
+}, "0.1", {"requires": [
+ "node", "dom", "io", "widget", "array-extras",
+ "transition", "lp.registry.distroseries.widgets",
+ "lp.app.formwidgets", "lp.app.picker", "lp.extras"]});
=== modified file 'lib/lp/registry/javascript/distroseries/tests/test_initseries.html'
--- lib/lp/registry/javascript/distroseries/tests/test_initseries.html 2011-08-08 08:40:51 +0000
+++ lib/lp/registry/javascript/distroseries/tests/test_initseries.html 2011-08-09 16:26:11 +0000
@@ -34,6 +34,10 @@
<script type="text/javascript"
src="../../../../app/javascript/extras/extras.js"></script>
<script type="text/javascript"
+ src="../../../../app/javascript/effects/effects.js"></script>
+ <script type="text/javascript"
+ src="../../../../app/javascript/expander.js"></script>
+ <script type="text/javascript"
src="../widgets.js"></script>
<!-- The module under test -->
=== modified file 'lib/lp/registry/javascript/distroseries/tests/test_initseries.js'
--- lib/lp/registry/javascript/distroseries/tests/test_initseries.js 2011-07-18 10:50:01 +0000
+++ lib/lp/registry/javascript/distroseries/tests/test_initseries.js 2011-08-09 16:26:11 +0000
@@ -12,8 +12,9 @@
var namespace = Y.namespace('lp.registry.distroseries.initseries.test');
- var Assert = Y.Assert;
- var ArrayAssert = Y.ArrayAssert;
+ var Assert = Y.Assert,
+ ArrayAssert = Y.ArrayAssert,
+ attrselect = Y.lp.extras.attrselect;
var suite = new Y.Test.Suite("distroseries.initseries Tests");
var initseries = Y.lp.registry.distroseries.initseries;
@@ -54,19 +55,28 @@
architectureChoice: {
get: function(name) {
Assert.areEqual("choice", name);
- return ["i386", "sparc"];
+ return [
+ {value: "i386", text: "i386"},
+ {value: "sparc", text: "sparc"}
+ ];
}
},
packagesetChoice: {
get: function(name) {
Assert.areEqual("choice", name);
- return ["4", "5"];
+ return [
+ {value: "4", text: "FooSet"},
+ {value: "5", text: "BarSet"}
+ ];
}
},
packageCopyOptions: {
get: function(name) {
Assert.areEqual("choice", name);
- return "Copy Source and Rebuild";
+ return {
+ value: "rebuild",
+ text: "Copy Source and Rebuild"
+ };
}
}
});
@@ -218,8 +228,8 @@
// The architecture picker features the architectures
// from the previous series.
ArrayAssert.itemsAreEqual(
- ['hppa', 'i386'],
- form_actions.architectureChoice.get("choices"));
+ ['hppa', 'i386'], attrselect("value")(
+ form_actions.architectureChoice.get("choices")));
}
};
@@ -228,5 +238,5 @@
}, "0.1", {"requires": [
'test', 'console', 'node-event-simulate',
- 'lp.app.formwidgets.test',
+ 'lp.app.formwidgets.test', 'lp.extras',
'lp.registry.distroseries.initseries']});
=== modified file 'lib/lp/registry/javascript/distroseries/tests/test_widgets.js'
--- lib/lp/registry/javascript/distroseries/tests/test_widgets.js 2011-08-05 08:54:55 +0000
+++ lib/lp/registry/javascript/distroseries/tests/test_widgets.js 2011-08-09 16:26:11 +0000
@@ -187,7 +187,7 @@
distro_arch_serieses_collection);
ArrayAssert.itemsAreEqual(
["amd64", "i386"],
- this.widget.get("choices"));
+ attrselect("value")(this.widget.get("choices")));
},
testAddDistroSeriesInitiatesIO: function() {
@@ -207,6 +207,7 @@
},
testRemoveDistroSeries: function() {
+ var values = attrselect("value");
var distro_arch_serieses1 = [
{architecture_tag: "i386"},
{architecture_tag: "amd64"},
@@ -220,7 +221,7 @@
distro_arch_serieses_collection1);
ArrayAssert.itemsAreEqual(
["amd64", "i386"],
- this.widget.get("choices"));
+ values(this.widget.get("choices")));
var distro_arch_serieses2 = [
{architecture_tag: "hppa"},
{architecture_tag: "hppa"},
@@ -234,11 +235,11 @@
distro_arch_serieses_collection2);
ArrayAssert.itemsAreEqual(
["amd64", "hppa", "i386"],
- this.widget.get("choices"));
+ values(this.widget.get("choices")));
this.widget.remove_distroseries("4");
ArrayAssert.itemsAreEqual(
["amd64", "i386"],
- this.widget.get("choices"));
+ values(this.widget.get("choices")));
},
testSetDistroSeriesError: function() {
@@ -283,18 +284,6 @@
);
},
- testAddChoice: function() {
- this.widget.add_choice({value: 'c', text: 'c', data: 'c'});
- ArrayAssert.itemsAreEqual(
- ["c"], this._getValues(this.widget.get("choices")));
- this.widget.add_choice({value: 'a', text: 'a', data: 'a'});
- ArrayAssert.itemsAreEqual(
- ["a", "c"], this._getValues(this.widget.get("choices")));
- this.widget.add_choice({value: 'b', text: 'b', data: 'b'});
- ArrayAssert.itemsAreEqual(
- ["a", "b", "c"], this._getValues(this.widget.get("choices")));
- },
-
testAddPackagesets: function() {
var package_sets = [
{id: "4", name: "foo", description: "Foo"},
@@ -313,23 +302,6 @@
attrselect("value")(choices));
},
- testAddPackagesetsCallsAutoSize: function() {
- var package_sets = [
- {name: "foo", description: "Foo"},
- {name: "bar", description: "Bar"},
- {name: "baz", description: "Baz"}
- ];
- var package_sets_collection =
- new Y.lp.client.Collection(
- null, {entries: package_sets}, null);
- var distroseries = {value: "4", title: "series1"};
- var autoSized = false;
- this.widget.autoSize = function() { autoSized = true; };
- this.widget.add_packagesets(
- package_sets_collection, distroseries);
- Assert.isTrue(autoSized);
- },
-
testAddDistroSeriesInitiatesIO: function() {
var io = false;
this.widget.client = {
@@ -467,7 +439,8 @@
};
testPackagesetPickerWidget = Y.merge(
- formwidgets.test.testSelectWidget, testPackagesetPickerWidget);
+ formwidgets.test.testChoiceListWidget,
+ testPackagesetPickerWidget);
suite.add(new Y.Test.Case(testPackagesetPickerWidget));
=== modified file 'lib/lp/registry/javascript/distroseries/widgets.js'
--- lib/lp/registry/javascript/distroseries/widgets.js 2011-07-22 10:19:51 +0000
+++ lib/lp/registry/javascript/distroseries/widgets.js 2011-08-09 16:26:11 +0000
@@ -444,7 +444,7 @@
/**
- * A special form of SelectWidget for choosing packagesets.
+ * A special form of ChoiceListWidget for choosing packagesets.
*
* @class PackagesetPickerWidget
*/
@@ -494,7 +494,29 @@
});
-Y.extend(PackagesetPickerWidget, formwidgets.SelectWidget, {
+Y.extend(PackagesetPickerWidget, formwidgets.ChoiceListWidget, {
+
+ /**
+ * Return whether this widget is displaying any choices.
+ *
+ * @method _hasChoices
+ * @return {Boolean}
+ */
+ _hasChoices: function() {
+ return this.fieldNode.one("li > input") !== null;
+ },
+
+ /**
+ * Display a message when no parent series are selected.
+ *
+ * @method _cleanDisplay
+ */
+ _cleanDisplay: function() {
+ if (!this._hasChoices()) {
+ var content = "<div>[No package sets to select from yet!]</div>";
+ this.fieldNode.empty().append(content);
+ }
+ },
/**
* Add a distroseries: add its packagesets to the packageset picker.
@@ -528,83 +550,6 @@
},
/**
- * Display a simple message when no parent series are selected.
- *
- * @method clean_display
- */
- clean_display: function() {
- /* XXX: GavinPanella 2011-07-20 bug=814531: The list of
- choices should be set via this.set("choices", ...) and the
- setter should ensure that the selection is maintained.
- Doing it via the back door means that the choicesChange
- event is not fired. Rather than fire it explicity we should
- DTRT. */
- /* XXX: GavinPanella 2011-07-18 bug=814535: This does not
- update this._distroseries and can thus leave the widget in
- an invalid state. */
- this.fieldNode.empty();
- this.fieldNode.append(Y.Node.create("<div />")
- .set('text', '[No package sets to select from yet!]'));
- this.renderUI();
- },
-
- /**
- * Initialize the picker's select node.
- *
- * @method init_select
- */
- init_select: function() {
- /* XXX: GavinPanella 2011-07-20 bug=814531: This method will
- not be necessary once this.set("choices", ...) is used
- consistently. */
- var select = this.fieldNode.one('select');
- if (select === null) {
- select = Y.Node.create("<select />");
- select.set("name", this.get("name"))
- .set("size", this.get("size"));
- if (this.get("multiple")) {
- select.set("multiple", "multiple");
- }
- this.fieldNode.empty().append(select);
- }
- return select;
- },
-
- /**
- * Add a choice to the picker.
- *
- * @method add_choice
- * @param {Object} choice The choice object to be added
- * ({text: choice_text, value: choice_value, data: choice_data}).
- */
- add_choice: function(choice) {
- /* XXX: GavinPanella 2011-07-20 bug=814531: The list of
- choices should be set via this.set("choices", ...) and the
- setter should ensure that the selection is maintained.
- Doing it via the back door means that the choicesChange
- event is not fired. Rather than fire it explicity we should
- DTRT. */
- var select = this.init_select();
- var option = Y.Node.create("<option />");
- option.set("value", choice.value)
- .set("text", choice.text)
- .setData("data", choice.data);
- var options = select.all('option');
- if (options.isEmpty()) {
- select.append(option);
- }
- else {
- var pos = this._sorted_position(choice.text);
- if (pos === 0) {
- select.prepend(option);
- }
- else {
- select.insertBefore(option, options.item(pos));
- }
- }
- },
-
- /**
* Add choices (a set of packagesets) to the picker.
*
* @method add_packagesets
@@ -615,21 +560,20 @@
*/
add_packagesets: function(packagesets, distroseries) {
this._packagesets[distroseries.value] = packagesets.entries;
- packagesets.entries.forEach(
- function(packageset) {
- var value = packageset.get("id");
- this.add_choice({
- data: packageset,
- value: value,
- text: (
- packageset.get("name") + ": " +
- packageset.get("description") +
- " (" + distroseries.title + ") ")
- });
- }, this);
- this.autoSize(10);
- Y.lazr.anim.green_flash({node: this.fieldNode}).run();
- },
+ var choices = packagesets.entries.map(function(packageset) {
+ return {
+ data: packageset,
+ value: packageset.get("id"),
+ text: Y.substitute(
+ "{name}: {desc} ({title})", {
+ name: packageset.get("name"),
+ desc: packageset.get("description"),
+ title: distroseries.title
+ })
+ };
+ });
+ this.add_choices(choices);
+ },
/**
* Remove a distroseries: remove its packagesets from the picker.
@@ -639,22 +583,13 @@
* removed.
*/
remove_distroseries: function(distroseries_id) {
- this._packagesets[distroseries_id].forEach(
- function(packageset) {
- /* XXX: GavinPanella 2011-07-20 bug=814531: The list
- of choices should be set via this.set("choices",
- ...) and the setter should ensure that the
- selection is maintained. Doing it via the back door
- means that the choicesChange event is not
- fired. Rather than fire it explicity we should
- DTRT. */
- this.fieldNode.one(
- 'option[value="' + packageset.get("id") + '"]').remove();
- }, this);
- Y.lazr.anim.green_flash({node: this.fieldNode}).run();
- if (this.fieldNode.all('option').isEmpty()) {
- this.clean_display();
- }
+ var packagesets = this._packagesets[distroseries_id];
+ /* When removing, we only need to construct choice objects
+ with a value. */
+ var choices = packagesets.map(function(packageset) {
+ return { value: packageset.get("id") };
+ });
+ this.remove_choices(choices);
},
initializer: function(config) {
@@ -662,12 +597,18 @@
this.error_handler = new Y.lp.client.ErrorHandler();
this.error_handler.clearProgressUI = Y.bind(this.hideSpinner, this);
this.error_handler.showError = Y.bind(this.showError, this);
- this.clean_display();
- // _packagesets maps each distroseries' id to a collection of ids
- // of its packagesets.
- // It's populated each time a new distroseries is added as a parent
- // and used when a distroseries is removed to get all the
- // corresponding packagesets to be removed from the widget.
+ /* Display a message if there are no choices. */
+ this.after("choicesChange", Y.bind(this._cleanDisplay, this));
+ this._cleanDisplay();
+ /* Green-flash when the set of choices is changed. */
+ this.after("choicesChange", function(e) {
+ Y.lazr.anim.green_flash({node: this.fieldNode}).run();
+ });
+ /* _packagesets maps each distroseries' id to a collection of
+ ids of its packagesets. It's populated each time a new
+ distroseries is added as a parent and used when a
+ distroseries is removed to get all the corresponding
+ packagesets to be removed from the widget. */
this._packagesets = {};
}
@@ -679,4 +620,4 @@
}, "0.1", {"requires": [
"node", "dom", "io", "widget", "lp.client",
"lp.app.formwidgets", "lazr.anim", "array-extras",
- "transition"]});
+ "substitute", "transition"]});
