Opened 13 years ago

Closed 13 years ago

Last modified 13 years ago

#2594 closed enhancement (wontfix)

Add multiselect configuration control

Reported by: mythtv@… Owned by: Isaac Richards
Priority: minor Milestone: unknown
Component: mythtv Version: head
Severity: medium Keywords:
Cc: devel@… Ticket locked: no

Description

This patch expands the functionality of the SelectSetting? class in order to provide multi-select capability.

The new implementation stores a multi-selection (from a QListBox) as a ‘;’ delimited string in the database. It preserves the order in which selections are made (in order to provide order preference) and treats single-selection functionality (e.g. from a QComboBox) as a special case of multi-selection.

The new implementation also provides an “expand” policing mechanism. This is in order to filter erroneous settings (usually from the database) from appearing in selections. Also to this end, it declares the “SetValue?()” member function as private so that the setting of a selection control to an arbitrary value is prohibited. When the system contains a selection option, for which a number of potential choices are available, and there is the ability to set arbitrary values (which ultimately may not match any of the expected selections) it can cause problems.

In order to facilitate a multi-selection widget, minor modification were made to the MythListBox? class. These result in Left/Right? action mapping to Select/Deselect? the current item (in a toggle manner). These actions used to cause navigation to the next/previous widget. I made this change for two reasons, one is that it is in keeping with the logic of most of the other widgets (namely, up/down cycles through widgets, and left/right changes values), but also I have found that using left/right for navigation often results in moving to the next widget and then changing its value (if it’s a widget that doesn’t support left/right navigation). I personally feel this provides a more uniform user experience (but would accept that this navigation is not ideal with long lists).

A few bugs, or potential bugs, were addressed along the way. There was a selection bug on first entering a MythListBox?. There were also situations where the selection displayed by the Qt widget could become out of sync with the underlying SelectSetting? state (mainly in situations where the state was updated programmatically for which signals indicating a selection change are not generated by the Qt widgets).

All of the other changes were required in order to overcome “SetValue?()” becoming private.

Attachments (21)

multiselect_mythtv.patch (47.9 KB) - added by mythtv@… 13 years ago.
multiselect_mythplugins.patch (1.4 KB) - added by mythtv@… 13 years ago.
multiselect_mythtv_omitted_0.1.patch (539 bytes) - added by mythtv@… 13 years ago.
Required change of "SetValue?()" call omitted from previous patches
multiselect_mythtv_r11685.patch (48.2 KB) - added by mythtv@… 13 years ago.
Patch refresh against head revision 11685 (also rolled in contents of “multiselect_mythtv_omitted_0.1.patch”)
multiselect_mythplugins_r11685.patch (1.4 KB) - added by mythtv@… 13 years ago.
Patch refresh against head revision 11685
multiselect_mythtv_r11735.patch (48.4 KB) - added by mythtv@… 13 years ago.
Patch refresh against head revision 11735
multiselect_mythplugins_r11735.patch (1.4 KB) - added by mythtv@… 13 years ago.
Patch refresh against head revision 11735
multiselect_mythtv_r11748.patch (48.4 KB) - added by mythtv@… 13 years ago.
Patch refresh against head revision 11748
multiselect_mythplugins_r11748.patch (1.4 KB) - added by mythtv@… 13 years ago.
Patch refresh against head revision 11748
multiselect_mythtv_r11772.patch (49.0 KB) - added by mythtv@… 13 years ago.
Patch refresh against head revision 11772
multiselect_mythplugins_r11772.patch (1.4 KB) - added by mythtv@… 13 years ago.
Patch refresh against head revision 11772
multiselect_mythtv_r11817.patch (49.6 KB) - added by mythtv@… 13 years ago.
Patch refresh against head revision 11817
multiselect_mythplugins_r11817.patch (1.4 KB) - added by mythtv@… 13 years ago.
Patch refresh against head revision 11817
multiselect_mythtv_r12159.patch (50.9 KB) - added by mythtv@… 13 years ago.
Patch refresh against head revision 12159
multiselect_mythplugins_r12159.patch (1.4 KB) - added by mythtv@… 13 years ago.
Patch refresh against head revision 12159
multiselect_mythtv_r12328.patch (51.2 KB) - added by mythtv@… 13 years ago.
Patch refresh against head revision 12328
multiselect_mythplugins_r12328.patch (1.4 KB) - added by mythtv@… 13 years ago.
Patch refresh against head revision 12328
multiselect_mythtv_r12631.patch (51.2 KB) - added by mythtv@… 13 years ago.
Patch refresh against head revision 12631
multiselect_mythtv_r12650.patch (51.6 KB) - added by mythtv@… 13 years ago.
Patch refresh against head revision 12650
multiselect_mythtv_r12703.patch (62.1 KB) - added by mythtv@… 13 years ago.
Patch refresh against head revision 12703
multiselect_mythtv_r12743.patch (52.3 KB) - added by mythtv@… 13 years ago.
Patch refresh against head revision 12743

Download all attachments as: .zip

Change History (33)

Changed 13 years ago by mythtv@…

Attachment: multiselect_mythtv.patch added

Changed 13 years ago by mythtv@…

Changed 13 years ago by mythtv@…

Required change of "SetValue?()" call omitted from previous patches

comment:1 Changed 13 years ago by John Poet <jppoet@…>

Just a note that commit 11681 breaks multiselect_mythtv.patch. Just a one line fix, but that patch should probably be updated.

John

Changed 13 years ago by mythtv@…

Patch refresh against head revision 11685 (also rolled in contents of “multiselect_mythtv_omitted_0.1.patch”)

Changed 13 years ago by mythtv@…

Patch refresh against head revision 11685

Changed 13 years ago by mythtv@…

Patch refresh against head revision 11735

Changed 13 years ago by mythtv@…

Patch refresh against head revision 11735

Changed 13 years ago by mythtv@…

Patch refresh against head revision 11748

Changed 13 years ago by mythtv@…

Patch refresh against head revision 11748

Changed 13 years ago by mythtv@…

Patch refresh against head revision 11772

Changed 13 years ago by mythtv@…

Patch refresh against head revision 11772

Changed 13 years ago by mythtv@…

Patch refresh against head revision 11817

Changed 13 years ago by mythtv@…

Patch refresh against head revision 11817

Changed 13 years ago by mythtv@…

Patch refresh against head revision 12159

Changed 13 years ago by mythtv@…

Patch refresh against head revision 12159

comment:2 Changed 13 years ago by anonymous

I have been using these patches since they were first posted, and have had no problems.

It seems like the author is having to post a new version about once a week just to keep up with changes to svn.

Is there any estimate on when these patches might become part of the svn trunk?

Changed 13 years ago by mythtv@…

Patch refresh against head revision 12328

Changed 13 years ago by mythtv@…

Patch refresh against head revision 12328

comment:3 Changed 13 years ago by jppoet@…

Pretty sure this is out-of-date. Again :(

Any chance of this getting merged into trunk?

Thanks,

John

Changed 13 years ago by mythtv@…

Patch refresh against head revision 12631

comment:4 Changed 13 years ago by jppoet@…

Unfortunately, I believe this is still out of sync, if firewire is enabled. We need to get Daniel hooked on this patch, then this would not happen :)

John

Changed 13 years ago by mythtv@…

Patch refresh against head revision 12650

comment:5 in reply to:  4 Changed 13 years ago by mythtv@…

Replying to jppoet@gmail.com:

Unfortunately, I believe this is still out of sync, if firewire is enabled. We need to get Daniel hooked on this patch, then this would not happen :)

John

Sorry, was getting lazy. Needless to say, I don't use the Firewire feature...

Changed 13 years ago by mythtv@…

Patch refresh against head revision 12703

Changed 13 years ago by mythtv@…

Patch refresh against head revision 12743

comment:6 Changed 13 years ago by paulh

This looks OK apart from using left/right key presses to toggle the selection because it means you have to scroll to the top or bottom of the list to move the focus to another widget which is a pain for big lists like those used in MythMusic and it actually breaks the search dialog used in MythMusic's metadata editor and probably other places too.

I've been testing it using select to toggle the selections and keeping the left/right keys as they. Any objections to doing it this way for now?

I do agree that it would be better to consistently use the same keys to change focus. My personal view is it would be better to use two keys, lets say left/right, to always change the focus not just in the settings but also in the themed dialogs. The select key should always activate the current widget, toggle selections in a listbox, toggle check boxes, activate buttons etc. The up/down keys should move the current items in lists, change the selection in combo's, toggle check boxes etc.

Maybe that's something to think about while doing the UI update?

comment:7 in reply to:  6 Changed 13 years ago by mythtv@…

Replying to paulh:

I appreciate that using the left/right keys to toggle the selection (and therefore not allowing them to be used to move focus to the next widget) can be an issue for very long lists. I decided to implement this method after some consideration. After investigating how other parts of the UI code functioned I concluded that the approach was not to use “select” to toggle controls, “select” generally seems to mean “ok, I’ve finished making changes (on that page)”, that is at least how it behaves for the majority of the widgets.

As the default was to use left/right to change the state of a control, I followed this approach for the list box. I concluded that for the multiselect object that I had added to the MythMusic configuration any system was unlikely to have such a long list of visualisations as to be prohibitive to easy navigation. For lists that are much longer, for example the “search music” list in MythMusic, acceptance of the item highlighted can be achieved using the “select” button (the dialogue box has an “OK” button but it is somewhat redundant).

I accept that an approach would be to have “left”/”right”/”up”/”down” navigate focus and “select” change state, but I personally feel that consistency throughout the UI is more important than the problems produced by making this change. I feel that any configuration issues that are produced by this change should have their design reconsidered to better fit with the chosen behaviour of the UI (if anyone is able to highlight such “problem” areas to me I would be happy to undertake any such redesign work).

If the decision was to change the way all of the other widgets work, so that for example, pressing “select” on a toggle widget changed its state, or pressing “select” on a combobox widget produced a drop-down list of options that could be scrolled through (more in keeping with the way X-windows/PC behaves) then I would be in favour of the approach you advocate in this case. But it appears to me that such an approach has been rejected, and as such, I feel that the behaviour as implemented in this patch is following the existing precedence for the UI behaviour.

comment:8 Changed 13 years ago by devel@…

Cc: devel@… added

Please take a look at ticket http://svn.mythtv.org/trac/ticket/3078 "Consistent behaviour of SELECT action across widgets"

Currently the SELECT button does different things depending on which widget you are using, and this patch makes it consistent across all widgets, acting as a toggle control or doing nothing for those widgets that don't toggle. I have been using this patch since I posted it and it certainly makes navigation easier!

comment:9 Changed 13 years ago by paulh

I've been testing this some more and I seem to be getting lots of message like these in the frontend log.

these are from the channel editor in mythtv-setup

2007-03-25 23:24:54.056 SelectSetting()::setState(0, 0) - invalid value
2007-03-25 23:24:54.057 SelectSetting()::setState(1, 0) - invalid value
2007-03-25 23:24:54.057 SelectSetting()::setState(2, 0) - invalid value
2007-03-25 23:24:54.057 SelectSetting()::setState(3, 0) - invalid value
2007-03-25 23:24:54.057 SelectSetting()::setState(4, 0) - invalid value
2007-03-25 23:24:54.057 SelectSetting()::setState(5, 0) - invalid value
2007-03-25 23:24:54.057 SelectSetting()::setState(6, 0) - invalid value
2007-03-25 23:24:54.057 SelectSetting()::setState(7, 0) - invalid value
2007-03-25 23:24:54.057 SelectSetting()::setState(8, 0) - invalid value
2007-03-25 23:24:54.057 SelectSetting()::setState(9, 0) - invalid value

also many similar to these this one is from the 'Input Connections' screen but most screens in mythtv-setup seem to produce them.

2007-03-25 23:27:02.105 SelectSetting(cardid)::setCurrentLabel(1) - invalid label

I know they are only warning messages but I think you need to do something to tidy these up before this gets committed.

comment:10 in reply to:  8 Changed 13 years ago by anonymous

Replying to devel@mrwire.co.uk:

Please take a look at ticket http://svn.mythtv.org/trac/ticket/3078 "Consistent behaviour of SELECT action across widgets"

Currently the SELECT button does different things depending on which widget you are using, and this patch makes it consistent across all widgets, acting as a toggle control or doing nothing for those widgets that don't toggle. I have been using this patch since I posted it and it certainly makes navigation easier!

I agree. It's in svn now :-)

comment:11 Changed 13 years ago by paulh

Resolution: wontfix
Status: newclosed

This patch isn't really required anymore. It was done to make selecting visuals in MythMusic easier but that's now done using a custom dialog.

comment:12 in reply to:  11 Changed 13 years ago by mythtv@…

Replying to paulh:

This patch isn't really required anymore. It was done to make selecting visuals in MythMusic easier but that's now done using a custom dialog.

Can I just highlight that this patch implements, what I consider, an improvement of the selection mechanism (namely of only being allow to set selection items to a valid selections, as opposed to being able to set them to arbitrary values). I appreciate that this patch involves modification to a number of different files, but I consider the modifications to be worthwhile.

If there is a requirement to produce a patch that does not contain the "multiselect" object, then I would be happy to do this.

Note: See TracTickets for help on using tickets.