Skip to content

Conversation

@jsubloom
Copy link
Contributor

@jsubloom jsubloom commented Oct 30, 2018

Prevents a book from being opened if it contains features incompatible with old versions of Bloom.

This pull request should target 4.4 and master.
Commit #1 is identical to the pull request targeting 4.3, so only Commit #2 is different.


This change is Reviewable

@jsubloom jsubloom changed the title Check for breaking features in books (BL-6661) Check for breaking features in books and write version requirements (BL-6661) (4.4+) Oct 30, 2018
Copy link
Contributor

@JohnThomson JohnThomson left a comment

Choose a reason for hiding this comment

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

One question and possible fix...also, it might be easier to merge the first commit into 4.3, then merge 4.3 into 4.4, and then do a 4.4 PR that only has the extra stuff. Having the same changes independently in 4.3 and 4.4 may make merging difficult...certainly will if they aren't exactly the same.

Reviewed 9 of 9 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @sujeffreyl and @hatton)


src/BloomExe/Book/HtmlDom.cs, line 1830 at r1 (raw file):

		public bool DoesContainNarrationAudioRecordedUsingWholeTextBox()
		{
			var nodes = _dom.SafeSelectNodes("//*[@data-audiorecordingmode='TextBox']");

I think it might be worth checking that the problem nodes actually have recordings. Or is it dangerous anyway? e.g., if they open a page that is marked TextBox in Bloom 4.3 (but has no recording) and record audio (at sentence level), it could come back to 4.4 with BOTH kinds of markup...what will 4.4 do then? Delete the sentence recordings?

Copy link
Contributor Author

@jsubloom jsubloom left a comment

Choose a reason for hiding this comment

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

Sure, i'll re-do another PR for 4.4 later using the proposed workflow

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @sujeffreyl and @hatton)


src/BloomExe/Book/HtmlDom.cs, line 1830 at r1 (raw file):

Previously, JohnThomson (John Thomson) wrote…

I think it might be worth checking that the problem nodes actually have recordings. Or is it dangerous anyway? e.g., if they open a page that is marked TextBox in Bloom 4.3 (but has no recording) and record audio (at sentence level), it could come back to 4.4 with BOTH kinds of markup...what will 4.4 do then? Delete the sentence recordings?

Maybe the best fix is 2 part:

  1. to only block if the nodes have actual audio recording
  2. If an acceptable book is opened, 4.3 should setAttribute data-audioRecordingMode to Sentence. I think this will be cleaner than having 4.4 versions and later attempt to recover.

Another question...
Do we care about 4.2 compatibility?

Copy link
Contributor

@JohnThomson JohnThomson left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @JohnThomson and @hatton)


src/BloomExe/Book/HtmlDom.cs, line 1830 at r1 (raw file):

Previously, sujeffreyl wrote…

Maybe the best fix is 2 part:

  1. to only block if the nodes have actual audio recording
  2. If an acceptable book is opened, 4.3 should setAttribute data-audioRecordingMode to Sentence. I think this will be cleaner than having 4.4 versions and later attempt to recover.

Another question...
Do we care about 4.2 compatibility?

I don't think so. Our expectation is that people who want to use the new feature will have 4.4. We're trying here to do the minimum that will prevent them disastrously shooting themselves in the foot if they try to work on a book with mixed versions. In view of that, it was probably unwise for me to suggest the added complication even if it wasn't dangerous unless there's a recording. Since it might be, all the more so.
There's not much we can do about 4.2 compatibility. By the time 4.4 goes beta and this situation is likely to affect users, 4.3 will be released. People with 4.2 who are updating will get 4.3, which has the warning. People who are not updating we can't protect. There's no upgrade path from 4.2 to a later 4.2 once we release 4.3.

@jsubloom
Copy link
Contributor Author

Closing without merging. Use #2855 instead which has cleaner version history with the changes made in 4.3.

@jsubloom jsubloom closed this Oct 31, 2018
@jsubloom jsubloom deleted the jeffrey_su/BL-6661_AudioRecordingMinVersion4.4 branch November 19, 2018 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants