Opened 13 years ago

Closed 13 years ago

Last modified 12 years ago

#2078 closed enhancement (fixed)

Better mythcontrols UI and reorganizes logic

Reported by: mfgalizi@… Owned by: danielk
Priority: minor Milestone: 0.21
Component: mythcontrols Version: head
Severity: low Keywords: mythcontrols ui logic
Cc: Ticket locked: no

Description

I'm not sure when, but not long after the release of 0.19 someone added some code to add alternate methods of browsing key bindings. A great idea, but the implementation looked poor and added alot of code to mythcontrols.cpp.

The attached patch removes most of that extra code (in favour of using the keybindings class, as was originally intended for this plugin) and improves (I think) the alternate views.

Deleting from the alternate views will be easier with these modifications, and maybe someone will think of a novel way to add bindings that works well too. I do plan on adding the code to facilitate deletion soon.

Attachments (8)

ui_and_logic.diff (40.4 KB) - added by mfgalizi@… 13 years ago.
The patch (no deletion)
ui_and_logic.2.diff (99.6 KB) - added by mfgalizi@… 13 years ago.
UI and Logic (with formatting)
standard_formatting.diff (102.6 KB) - added by mfgalizia 13 years ago.
Fixes mythcontrols to use MythTV coding standards
fix_remove.diff (1.5 KB) - added by mfgalizi 13 years ago.
Uses the correct context when removing keys from an action.
view_menu.diff (13.0 KB) - added by mfgalizia 13 years ago.
Uses a menu to change the views instead of "magic keys." This does not change the way that alternate views display information. This patch replaces the previous (fix_remove.diff) patch.
ui_and_logic_again.diff (36.5 KB) - added by mfgalizia 13 years ago.
Fixes the logic and ui (again)
2078-v1.patch (36.9 KB) - added by danielk 13 years ago.
Cleanup of ui_and_logic_again.diff
2078-v2.patch (37.6 KB) - added by mfgalizi 13 years ago.
Fixes problems still present in v1

Download all attachments as: .zip

Change History (18)

Changed 13 years ago by mfgalizi@…

Attachment: ui_and_logic.diff added

The patch (no deletion)

comment:1 Changed 13 years ago by danielk

Milestone: 0.21
Owner: changed from Isaac Richards to danielk
Version: head

Micah, I'll have a look at this after the release. But in the meantime please reformat the additions in the MythTV coding style. A separate reformating patch for the existing code would be welcomed as well.

comment:2 Changed 13 years ago by danielk

Resolution: invalid
Status: newclosed

needs reformatting

comment:3 Changed 13 years ago by mfgalizi@…

Resolution: invalid
Status: closedreopened

OK, formatting of the entire plugin is fixed with this patch. This also has the modifications that the last patch had.

Changed 13 years ago by mfgalizi@…

Attachment: ui_and_logic.2.diff added

UI and Logic (with formatting)

comment:4 Changed 13 years ago by danielk

Resolution: invalid
Status: reopenedclosed

The reformatting of existing code needs to be in a separate patch from new code additions and functional modifications.

Changed 13 years ago by mfgalizia

Attachment: standard_formatting.diff added

Fixes mythcontrols to use MythTV coding standards

comment:5 Changed 13 years ago by mfgalizia

Resolution: invalid
Status: closedreopened

comment:6 Changed 13 years ago by danielk

(In [11515]) Refs #2078. Cleanup of MythControls?. Should not change functionality.

This applies Micah's cleanup patch + additional cleanups + adds plugins to the doxygen documentation.

comment:7 Changed 13 years ago by otto at kolsi dot fi

I'm having following problems with SVN 11533:

When trying to remove e.g. key 'O' / TOGGLEBROWSE from the TV Playback context, mythcontrols mixes the context. It shows dialog: "Confirm - Delete this key binding from context Music?". If Confirm is selected, there's an error about telling that this is mandatory action.

Following shows this from the DB:

mysql> select context, action, keylist from keybindings where keylist = 'O';
+-------------+--------------+---------+
| context     | action       | keylist |
+-------------+--------------+---------+
| TV Frontend | UPCOMING     | O       |
| TV Playback | TOGGLEBROWSE | O       |
| Music       | STOP         | O       |
+-------------+--------------+---------+

If I go to Music, I can delete the 'O' binding from there and it really removes the Music binding.

Even if I add a second key binding to TV Playback, TV Frontend and Music, mythcontrols still won't let me delete the 'O' keybinding from the TV Playback. It says it's mandatory action.

mysql> select context, action, keylist from keybindings where keylist 
like '%O,%';
+-------------+--------------+----------+
| context     | action       | keylist  |
+-------------+--------------+----------+
| TV Frontend | UPCOMING     | O,Ctrl+D |
| TV Playback | TOGGLEBROWSE | O,Ctrl+A |
| Music       | STOP         | O,Ctrl+D |
+-------------+--------------+----------+

Just to verify that there are no actions that have only the 'O' mapping, which would justify the "mandatory error message":

mysql> select context, action, keylist from keybindings where keylist = 
'O'; 

Empty set (0.00 sec)

So it seems to get confused from which context user want's to delete a key. And also fails to correctly identify a mandatory action.

Previous example is not the only one that fails for me, just an example.

comment:8 Changed 13 years ago by mfgalizia

The problem is with Binding class, which shouldn't ever have been added. I will attach a patch to fix the problem.

Changed 13 years ago by mfgalizi

Attachment: fix_remove.diff added

Uses the correct context when removing keys from an action.

Changed 13 years ago by mfgalizia

Attachment: view_menu.diff added

Uses a menu to change the views instead of "magic keys." This does not change the way that alternate views display information. This patch replaces the previous (fix_remove.diff) patch.

Changed 13 years ago by mfgalizia

Attachment: ui_and_logic_again.diff added

Fixes the logic and ui (again)

Changed 13 years ago by danielk

Attachment: 2078-v1.patch added

Cleanup of ui_and_logic_again.diff

comment:9 Changed 13 years ago by danielk

There are still problems with 2078-v1.patch:

The "Context By Keys" view has "Context" in the title above the list of keys, and "Actions" above the list of contexts. Also it allows you to enter a key binding when you select the context.

The "Keys By Context" view shows keys in the action pane, not actions.

The Change View menu only shows the other views. This is confusing, it should show all three views in the same order, but grey out the button for the current view. Some people have much better spatial memory than textual memory.

When you exit the key editor it crashes in UpdateRightList?() when m_leftList->GetItemCurrent?() returns null. (I've sent a backtrace to Micah for this.)

Changed 13 years ago by mfgalizi

Attachment: 2078-v2.patch added

Fixes problems still present in v1

comment:10 Changed 13 years ago by danielk

Resolution: fixed
Status: reopenedclosed

(In [13146]) Fixes #2078. Applies Micah's patch to improve MythControls? UI and reorganize the logic for the views.

The only user visible change are formatting changes and a menu for switching views, but this also cleans up how the views are handled internally. The extra views are still informative only.

Note: See TracTickets for help on using tickets.