Skip to content

Add separate control for full bleed layout#7657

Merged
andrew-polk merged 1 commit intoVersion6.3from
fullBleedControl
Feb 5, 2026
Merged

Add separate control for full bleed layout#7657
andrew-polk merged 1 commit intoVersion6.3from
fullBleedControl

Conversation

@JohnThomson
Copy link
Contributor

@JohnThomson JohnThomson commented Feb 4, 2026


Open with Devin

This change is Reviewable

@JohnThomson JohnThomson marked this pull request as draft February 4, 2026 19:42
Copy link

@devin-ai-integration devin-ai-integration bot left a comment

Choose a reason for hiding this comment

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

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 5 additional flags.

Open in Devin Review

@JohnThomson JohnThomson marked this pull request as ready for review February 4, 2026 19:55
Copy link
Contributor

@andrew-polk andrew-polk left a comment

Choose a reason for hiding this comment

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

@andrew-polk reviewed 5 files and all commit messages, and made 3 comments.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @JohnThomson).


DistFiles/localization/en/BloomMediumPriority.xlf line 778 at r1 (raw file):

        <source xml:lang="en">Replace the front cover content with a single full-bleed image. See [Full Page Cover Images](https://docs.bloomlibrary.org/full-page-cover-images) for information on sizing your image to fit.</source>
        <note>BookSettings.CoverIsImage.Description.V2</note>
      </trans-unit>

Should v1 above be marked obsolete? Or not yet because we aren't sure we are going with this approach?
Seems like we might as well then everything can be backed out together if we decide to revert.


src/BloomExe/Book/Book.cs line 4053 at r1 (raw file):

        public bool FullBleed =>
            PageSizeSupportsFullBleed()
            && BookInfo.AppearanceSettings.FullBleed

Are we deciding for now not to handle migration? Do we have a note about that somewhere?


src/BloomExe/Book/Book.cs line 4644 at r1 (raw file):

            
            // Device sizes do not support full bleed
            return !sizeName.StartsWith("Device", System.StringComparison.OrdinalIgnoreCase);

This isn't quite accurate. It should be kept in sync with ConfigureFullBleedPageSize.

Copy link
Contributor

@andrew-polk andrew-polk left a comment

Choose a reason for hiding this comment

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

@andrew-polk made 1 comment.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @JohnThomson).


src/BloomExe/Book/Book.cs line 4641 at r1 (raw file):

        {
            var layout = GetLayout();
            var sizeName = layout?.SizeAndOrientation?.PageSizeName ?? "A5";

If we can't determine page size, it seems like we should return false?

Copy link
Contributor Author

@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.

@JohnThomson made 4 comments.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @andrew-polk).


DistFiles/localization/en/BloomMediumPriority.xlf line 778 at r1 (raw file):

Previously, andrew-polk wrote…

Should v1 above be marked obsolete? Or not yet because we aren't sure we are going with this approach?
Seems like we might as well then everything can be backed out together if we decide to revert.

Done.


src/BloomExe/Book/Book.cs line 4053 at r1 (raw file):

Previously, andrew-polk wrote…

Are we deciding for now not to handle migration? Do we have a note about that somewhere?

I noted in the card several issues that I did not address, since John indicated that he wants to quickly get this working and see if it helps solve various problems. Effects on 6.2 was one of them.


src/BloomExe/Book/Book.cs line 4641 at r1 (raw file):

Previously, andrew-polk wrote…

If we can't determine page size, it seems like we should return false?

I suppose so. Weird case; we always set some page size, IIRC.


src/BloomExe/Book/Book.cs line 4644 at r1 (raw file):

Previously, andrew-polk wrote…

This isn't quite accurate. It should be kept in sync with ConfigureFullBleedPageSize.

I think the AI did it, and I went along because I thought we now handled all the others. I guess that's 6.4...or maybe I only got part way to handling them all. Done.

Copy link

@devin-ai-integration devin-ai-integration bot left a comment

Choose a reason for hiding this comment

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

Devin Review found 4 new potential issues.

View 4 additional findings in Devin Review.

Open in Devin Review

Copy link
Contributor

@andrew-polk andrew-polk left a comment

Choose a reason for hiding this comment

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

@andrew-polk reviewed 4 files and all commit messages, and resolved 4 discussions.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @JohnThomson).

@andrew-polk andrew-polk merged commit d57dfa2 into Version6.3 Feb 5, 2026
2 checks passed
@andrew-polk andrew-polk deleted the fullBleedControl branch February 5, 2026 21:11
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