Bug 384471

Summary: Media type selection for Image burning is partly wrong, comments are very wrong
Product: [Applications] k3b Reporter: Thomas Schmitt <scdbackup>
Component: generalAssignee: k3b developers <k3b>
Status: RESOLVED FIXED    
Severity: normal CC: michalm, simonandric5, trueg, zhaixiang
Priority: NOR    
Version: unspecified   
Target Milestone: ---   
Platform: Other   
OS: Linux   
Latest Commit: Version Fixed In:
Sentry Crash Report:

Description Thomas Schmitt 2017-09-07 17:05:43 UTC 
Hi,

K3b::Iso9660ImageWritingJob::startWriting() inapropriately accepts DVD+R for
m_writingMode == K3b::WritingModeRestrictedOverwrite :

  https://cgit.kde.org/k3b.git/tree/libk3b/jobs/k3biso9660imagewritingjob.cpp#n261
    else if( m_writingMode == K3b::WritingModeRestrictedOverwrite ) {
        mt = K3b::Device::MEDIA_DVD_PLUS_R|K3b::Device::MEDIA_DVD_PLUS_R_DL|K3b::Device::MEDIA_DVD_PLUS_RW|K3b::Device::MEDIA_DVD_RW_OVWR;

Restricted Overwrite a speciality of formatted DVD-RW.
"Overwrite" like with DVD-RAM, DVD+RW, BD-RE. "Resctricted" because only
full chunks of 32 KiB may be written.

The allowed gestures are also allowed with DVD+RW, DVD-RAM, BD-RE.
But it is not compatible with DVD+R or DVD+R DL.

So, after due checking whether WritingModeRestrictedOverwrite really
means what its name suggests, all types except formatted DVD-RW should be
removed from the "mt" list.
If one wants to allow DVD+RW, then also DVD-RAM and BD-RE should be allowed.

(In general i wonder why K3B already knows the future write type but not the
 medium type yet. Must be a GUI precognition thing.)

--------------------------------------------------------------------------

The comment in line 236 ff. is obviously outdated. The choice of wanted DVD
(and now BD) does not depend on the use of growisofs. The comment lists 10
decisions or remarks:

1. Ok. (but obviously writing app is not auto)
2. Wrong. The code looks for cdrdao as writing app, not for "not growisofs".
3. Wrong. If not cdrdao and not d->isDvdImage, then all media (including BD)
4. Ok.
5. No such code is to see.
6. No such code is to see.
7. No such code is to see.
8. No such code is to see.
9. Wrong. DVD+R and DVD+R DL are accepted inappropiately.
          DVD+RW is ok, but misplaced here, nevertheless.
10. obsolete, now that Bug 381074 is fixed.

In the code i see currently these five decisions:

1. If special CD features are requested: CD types only
   Special are: K3b::WritingAppCdrdao , K3b::WritingModeTao , WritingModeRaw
2. If formatted DVD-RW is requested: formatted DVD-RW, DVD+R, DVD+R DL, DVD+RW
   Request is: K3b::WritingModeRestrictedOverwrite
3. If image is larger than 900 MiB (d->isDvdImage == true): DVD or BD types
   See K3b::Iso9660ImageWritingJob::start()
4. If image not larger than 900 MiB: All media types
5. If not decided yet: All media types

After fixing my objection above, number 2 should become

2. If formatted DVD-RW is requested: formatted DVD-RW only
   Request is: K3b::WritingModeRestrictedOverwrite

Have a nice day :)

Thomas
Comment 1 Thomas Schmitt 2017-09-07 17:11:43 UTC 
Hi,

already the first bug in my bug report. Implemented decision 5 is indeed:

5. If not decided yet: DVD and BD media types.

This seems to assume that all CD use cases are completely covered by the
previous decisions.
If i was in charge i would allow all media types and let the media waiter
decide by the requested size and the size of the inserted media type.

Have a nice day :)

Thomas
Comment 2 Leslie Zhai 2017-09-08 03:06:21 UTC 
Git commit e2a5e4d95c228cda313ad6dadffb23648c05081a by Leslie Zhai.
Committed on 08/09/2017 at 03:05.
Pushed by lesliezhai into branch 'master'.

Thomas please review it.

M  +18   -21   libk3b/jobs/k3biso9660imagewritingjob.cpp

https://commits.kde.org/k3b/e2a5e4d95c228cda313ad6dadffb23648c05081a
Comment 3 Thomas Schmitt 2017-09-08 11:36:07 UTC 
Hi,

looks ok for me, besides another mistake by me:

K3b::WritingAppCdrdao is not a complete criterion for the need for CD.
The criterion in the code is distributed over two if-statements.
As one statement it is:

  (m_writingMode == K3b::WritingModeAuto ||
   m_writingMode == K3b::WritingModeSao) &&
   writingApp() == K3b::WritingAppCdrdao

So comment 1 should become:

  1. If special CD features are requested: CD types only Special are:
     K3b::WritingAppCdrdao with K3b::WritingModeAuto or K3b::WritingModeSao,
     any WritingApp with K3b::WritingModeTao,
     any WritingApp with K3b::WritingModeRaw


Have a nice day :)

Thomas
Comment 4 Leslie Zhai 2017-09-11 02:26:58 UTC 
Git commit 5051e3b80f66d4dc1308a8bbef54b845b8c54559 by Leslie Zhai.
Committed on 11/09/2017 at 02:26.
Pushed by lesliezhai into branch 'master'.

Update comment.

M  +3    -1    libk3b/jobs/k3biso9660imagewritingjob.cpp

https://commits.kde.org/k3b/5051e3b80f66d4dc1308a8bbef54b845b8c54559
Comment 5 Thomas Schmitt 2017-09-11 07:52:04 UTC 
Hi,

the new commit looks good to me.

If we do not jump into the adventure of verifying the implicit assumptions
about media types and write modes, then this bug report can be closed.

Have a nice day :)

Thomas