Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#8564 closed defect (fixed)

Inserting a new cut point alters existing cut points

Reported by: jrlagrone@… Owned by: sphery
Priority: minor Milestone: 0.24
Component: MythTV - General Version: Master Head
Severity: low Keywords:
Cc: stuartm Ticket locked: no

Description

When editing a file that already has cut points in it, adding a new cut point can remove the next or previous cut point.

To reproduce, add a new, delete after this point before an existing cut (last tested in r25101)

ie, so you have a file with cut points positioned like so:

.......[...].....

if you add an starting point for a cut before your already cut section like so:

...[...[...]....

you end up with a cut section that looks like this:

...[.......]....

Similarly adding a delete before cut point after a cut section or flipping directions of cut points has a similar effect.

I'm not positive, but this also seems to be making commercial flagging a bit less accurate, though that may just be the result of a few uncooperative recordings

Attachments (3)

8564.patch (3.8 KB) - added by Jim Stichnoth <stichnot@…> 9 years ago.
8564_v2.patch (3.9 KB) - added by Jim Stichnoth <stichnot@…> 9 years ago.
8564_v3.patch (4.7 KB) - added by Jim Stichnoth <stichnot@…> 9 years ago.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 9 years ago by markk

Owner: changed from Isaac Richards to markk
Status: newaccepted

comment:2 Changed 9 years ago by stuartm

Cc: stuartm added

comment:3 Changed 9 years ago by Jim Stichnoth <stichnot@…>

There is a logic error in DeleteMap::CleanMap?() :

Index: libs/libmythtv/deletemap.cpp
===================================================================
--- libs/libmythtv/deletemap.cpp        (revision 25143)
+++ libs/libmythtv/deletemap.cpp        (working copy)
@@ -387,7 +387,7 @@
             uint64_t thisframe = it.key();
             if (lasttype == thistype)
             {
-                Delete(thistype == MARK_CUT_END ? thisframe :
+                Delete(thistype == MARK_CUT_START ? thisframe :
                                                   (uint64_t)lastframe);
                 clear = false;
                 break;

I believe this fixes jrlagrone's examples.

However, the aggressive calling of CleanMap?() after every edit is a change from the old behavior. If I want to move the boundary of an existing cut region to the current point, I press Select, then Up or Down, and Select again, but once in a while the first Select is received as two separate Selects, which ends up deleting an adjacent cut mark without the opportunity to undo.

Changed 9 years ago by Jim Stichnoth <stichnot@…>

Attachment: 8564.patch added

comment:4 Changed 9 years ago by Jim Stichnoth <stichnot@…>

The attached patch restores the original behavior as far as I can tell.

  1. Fixes the logic error pointed out in the previous comment.
  1. Only calls DeleteMap::CleanMap?() after a cutlist is loaded and before it is saved.
  1. Doesn't try to clean the deletemap when Add() is called.
  1. This is the tricky part. In OSD::SetRegions?(), convert discontinuities (i.e., two marks in a row of the same type) into overlapping cut regions. It would probably be cleaner to extend the definition of regions in MythUIEditBar to include whether the start point and the end point should contain a start/end image, but that would require a lot more than a one-line change to MythUIEditBar.

comment:5 Changed 9 years ago by sphery

Owner: changed from markk to sphery

Changed 9 years ago by Jim Stichnoth <stichnot@…>

Attachment: 8564_v2.patch added

Changed 9 years ago by Jim Stichnoth <stichnot@…>

Attachment: 8564_v3.patch added

comment:6 Changed 9 years ago by Jim Stichnoth <stichnot@…>

Uploaded v3 patch, which adds some code to avoid editbar anomalies by ensuring that the individual segments have width 1 or more.

comment:7 Changed 9 years ago by sphery

Resolution: fixed
Status: acceptedclosed

(In [26111]) Refactor the user interface of the recording cut list editor to take advantage of the new cleaner design of the editor provided by Mark's libmythui-osd code, which ensures users can't create invalid cut lists with discontinuities. Refs #7916. Fixes #8564. Fixes #8832.

Includes some changes to key bindings to make key usage more consistent with the rest of MythTV.

 * MENU no longer exits the editor.  It now brings up the menu.
 * SELECT no longer brings up the menu.  It now adds a new cut or, when in a cut area, prompts whether to move the cut start/end or delete the cut.
 * EDIT exits the editor and saves the cut list.
 * ESCAPE prompts whether to save the cut list or cancel changes, then exits.
 * DELETE can be used to delete a cut, when inside a cut area.
 * SAVEMAP (new binding, no default) saves the cut list without exiting (works well when mapped to the same key as TV Editing/TOGGLERECORD).

The menu allows moving the previous/next cut point to the current position or, if there is no previous/next cut point, cutting to the beginning or end of the recording, and allows adding a new cut. It also includes an option, "Cut List Options", which brings up a secondary menu allowing the user to "Clear Cut List", "Invert Cut List", "Undo Changes" (revert to the saved cut list), "Exit Without Saving", "Save Cut List" (without exiting), or "Save Cut List and Exit".

A typical editing session would involve:

 * Hit EDIT (E) to enter edit mode
 * Optionally import the flag list with LOADCOMMSKIP (Z)
 * Find the start (or end) frame for a new cut and press SELECT (Space/Enter)
 * Find the end (or start) frame for that cut and press SELECT (Space/Enter)
 * To adjust positions of existing cuts, find the desired frame, then press MENU (M), then select "Move Previous Cut End Here" or "Move Next Cut Start Here"
 * To remove a cut, press DELETE (D) inside the cut region
 * Hit EDIT (E) to exit edit mode and save the cut list

All actions could instead be performed with the MENU (M).

Thanks to Jim Stichnoth for the MythUIEditBar fixes and for the patches on #8564 that people have been using while I've been working on this patch.

comment:8 Changed 9 years ago by sphery

(In [26113]) Fix a copy/paste error in a method declaration from [26111]. Refs #8564.

comment:9 Changed 9 years ago by sphery

(In [26114]) Fix temporary placeholder handling after [26111], making them non-persistent. Refs #8564.

comment:10 Changed 9 years ago by stuartm

Milestone: unknown0.24
Note: See TracTickets for help on using tickets.