Skip to content

Allow selected_index=none in selection container widgets #1495

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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
0376350
Allow none in accordion widget
pbugnion Jul 11, 2017
9b371aa
Allow none in tab panel
pbugnion Jul 11, 2017
b6ec556
Add framework for current selection unit tests
pbugnion Jul 11, 2017
1f77604
Test signal triggered correctly
pbugnion Jul 12, 2017
f481d54
Test for setting by value
pbugnion Jul 12, 2017
52d8e26
Test for setting index and values to wrong values
pbugnion Jul 12, 2017
98e48b6
Unittest for inserting an item in selection
pbugnion Jul 13, 2017
88e4182
Test for adjusting selection after items moved
pbugnion Jul 13, 2017
1a4a019
Test for adjusting selection after moving item
pbugnion Jul 13, 2017
465c1dd
Test for clearing selection
pbugnion Jul 13, 2017
a184d23
Tests for changing the item selected
pbugnion Jul 13, 2017
89238bb
Use ArrayExt.insert instead of splice.
pbugnion Jul 13, 2017
8bd2304
Actually modify items in place
pbugnion Jul 13, 2017
e170bb2
Tests for removing an item
pbugnion Jul 13, 2017
04c10b0
Long line fix
pbugnion Jul 13, 2017
eac8ae4
Validate that selected index is not out of bounds
pbugnion Jul 13, 2017
e5b298c
Appropriate file header
pbugnion Jul 13, 2017
59fcdb4
Update documentation for Tabs and Accordions
pbugnion Jul 13, 2017
88ba725
Explain selected_index traitlet
pbugnion Jul 13, 2017
c31ac16
fix typo
jasongrout Jul 14, 2017
a2af8b5
We don't allow an index out of range now, so update docs.
jasongrout Jul 14, 2017
cc0d435
Code style
jasongrout Jul 14, 2017
34dd60a
Translate from phosphor convention of -1 being no selection
jasongrout Jul 15, 2017
95cc187
Fix js comment
jasongrout Jul 15, 2017
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
4 changes: 2 additions & 2 deletions docs/source/examples/Widget List.ipynb
Original file line number Diff line number Diff line change
Expand Up @@ -1071,7 +1071,7 @@
"\n",
"Unlike the rest of the widgets discussed earlier, the container widgets `Accordion` and `Tab` update their `selected_index` attribute when the user changes which accordion or tab is selected. That means that you can both see what the user is doing *and* programmatically set what the user sees by setting the value of `selected_index`.\n",
"\n",
"Setting `selected_index = -1` (or any value outside the range of available accordions or tabs) closes all of the accordions or deselects all tabs."
"Setting `selected_index = None` closes all of the accordions or deselects all tabs."
]
},
{
Expand Down Expand Up @@ -1100,7 +1100,7 @@
},
"outputs": [],
"source": [
"accordion.selected_index = -1"
"accordion.selected_index = None"
]
},
{
Expand Down
28 changes: 28 additions & 0 deletions ipywidgets/widgets/tests/test_selectioncontainer.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
# Copyright (c) Jupyter Development Team.
# Distributed under the terms of the Modified BSD License.

from unittest import TestCase

from traitlets import TraitError

from ipywidgets.widgets import Accordion, HTML


class TestAccordion(TestCase):

def setUp(self):
self.children = [HTML('0'), HTML('1')]

def test_selected_index_none(self):
accordion = Accordion(self.children, selected_index=None)
state = accordion.get_state()
assert state['selected_index'] is None

def test_selected_index(self):
accordion = Accordion(self.children, selected_index=1)
state = accordion.get_state()
assert state['selected_index'] == 1

def test_selected_index_out_of_bounds(self):
with self.assertRaises(TraitError):
Accordion(self.children, selected_index=-1)
17 changes: 15 additions & 2 deletions ipywidgets/widgets/widget_selectioncontainer.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,27 @@

from .widget_box import Box, register
from .widget_core import CoreWidget
from traitlets import Unicode, Dict, CInt
from traitlets import Unicode, Dict, CInt, TraitError, validate
from ipython_genutils.py3compat import unicode_type


class _SelectionContainer(Box, CoreWidget):
"""Base class used to display multiple child widgets."""
_titles = Dict(help="Titles of the pages").tag(sync=True)
selected_index = CInt(help="The index of the selected page.").tag(sync=True)
selected_index = CInt(
help="""The index of the selected page.

This is either an integer selecting a particular sub-widget,
or None to have no widgets selected.""",
allow_none=True
).tag(sync=True)

@validate('selected_index')
def _validated_index(self, proposal):
if proposal.value is None or 0 <= proposal.value < len(self.children):
return proposal.value
else:
raise TraitError('Invalid selection: index out of bounds')

# Public methods
def set_title(self, index, title):
Expand Down
39 changes: 22 additions & 17 deletions packages/controls/src/phosphor/currentselection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ class Selection<T> {
*/
set value(value: T) {
if (value === null) {
this.index = -1;
this.index = null;
} else {
this.index = ArrayExt.firstIndexOf(this._array, value);
}
Expand All @@ -103,9 +103,9 @@ class Selection<T> {
* Get the index of the currently selected item.
*
* #### Notes
* This will be `-1` if no item is selected.
* This will be `null` if no item is selected.
*/
get index(): number {
get index(): number | null {
return this._index;
}

Expand All @@ -115,14 +115,19 @@ class Selection<T> {
* @param index - The index to select.
*
* #### Notes
* If the value is out of range, the index will be set to `-1`, which
* If the value is out of range, the index will be set to `null`, which
* indicates no item is selected.
*/
set index(index: number) {
set index(index: number | null) {
// Coerce the value to an index.
let i = Math.floor(index);
if (i < 0 || i >= this._array.length) {
i = -1;
let i;
if (index !== null) {
i = Math.floor(index);
if (i < 0 || i >= this._array.length) {
i = null;
}
} else {
i = null;
}

// Bail early if the index will not change.
Expand Down Expand Up @@ -194,7 +199,7 @@ class Selection<T> {

// Handle the behavior where the new item is always selected,
// or the behavior where the new item is selected if needed.
if (bh === 'select-item' || (bh === 'select-item-if-needed' && ci === -1)) {
if (bh === 'select-item' || (bh === 'select-item-if-needed' && ci === null)) {
this._index = i;
this._value = item;
this._previousValue = cv;
Expand Down Expand Up @@ -240,19 +245,19 @@ class Selection<T> {
let pv = this._value;

// Reset the current index and previous item.
this._index = -1;
this._index = null;
this._value = null;
this._previousValue = null;

// If no item was selected, there's nothing else to do.
if (pi === -1) {
if (pi === null) {
return;
}

// Emit the current changed signal.
this._selectionChanged.emit({
previousIndex: pi, previousValue: pv,
currentIndex: -1, currentValue: null
currentIndex: this._index, currentValue: this._value
});
}

Expand Down Expand Up @@ -283,12 +288,12 @@ class Selection<T> {
// No item gets selected if the vector is empty.
if (this._array.length === 0) {
// Reset the current index and previous item.
this._index = -1;
this._index = null;
this._value = null;
this._previousValue = null;
this._selectionChanged.emit({
previousIndex: i, previousValue: item,
currentIndex: -1, currentValue: null
currentIndex: this._index, currentValue: this._value
});
return;
}
Expand Down Expand Up @@ -334,12 +339,12 @@ class Selection<T> {
}

// Otherwise, no item gets selected.
this._index = -1;
this._index = null;
this._value = null;
this._previousValue = null;
this._selectionChanged.emit({
previousIndex: i, previousValue: item,
currentIndex: -1, currentValue: null
currentIndex: this._index, currentValue: this._value
});
}

Expand All @@ -348,7 +353,7 @@ class Selection<T> {
*/
private _updateSelectedValue() {
let i = this._index;
this._value = i !== -1 ? this._array[i] : null;
this._value = i !== null ? this._array[i] : null;
}

private _array: ReadonlyArray<T> = null;
Expand Down
14 changes: 8 additions & 6 deletions packages/controls/src/phosphor/tabpanel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -120,20 +120,22 @@ class TabPanel extends Widget {
* Get the index of the currently selected tab.
*
* #### Notes
* This will be `-1` if no tab is selected.
* This will be `null` if no tab is selected.
*/
get currentIndex(): number {
return this.tabBar.currentIndex;
get currentIndex(): number | null {
const currentIndex = this.tabBar.currentIndex;
// Phosphor tab bars have an index of -1 if no tab is selected
return (currentIndex === -1 ? null : currentIndex);
}

/**
* Set the index of the currently selected tab.
*
* #### Notes
* If the index is out of range, it will be set to `-1`.
* If the index is out of range, it will be set to `null`.
Copy link
Member

Choose a reason for hiding this comment

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

It looks like the logic here is still missing for out of range values.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, you don't need to do anything here - the tab bar handles the logic to set its current index to -1 if out of range. We'll just handle the translation to null up where we retrieve the index.

*/
set currentIndex(value: number) {
this.tabBar.currentIndex = value;
set currentIndex(value: number | null) {
this.tabBar.currentIndex = (value === null ? -1 : value);
}

/**
Expand Down
4 changes: 2 additions & 2 deletions packages/controls/src/widget_selectioncontainer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ class AccordionView extends DOMWidgetView {
// tabs before updating so we don't get spurious changes in the index,
// which would then set off another sync cycle.
this.updatingChildren = true;
this.pWidget.selection.index = -1;
this.pWidget.selection.index = null;
this.children_views.update(this.model.get('children'));
this.update_selected_index();
this.updatingChildren = false;
Expand Down Expand Up @@ -339,7 +339,7 @@ class TabView extends DOMWidgetView {
// tabs before updating so we don't get spurious changes in the index,
// which would then set off another sync cycle.
this.updatingTabs = true;
this.pWidget.currentIndex = -1;
this.pWidget.currentIndex = null;
this.childrenViews.update(this.model.get('children'));
this.pWidget.currentIndex = this.model.get('selected_index');
this.updatingTabs = false;
Expand Down
1 change: 1 addition & 0 deletions packages/controls/test/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,4 @@
// Distributed under the terms of the Modified BSD License.

import './widget_date_test';
import './phosphor/currentselection_test';
Loading