Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Chapter Indicator #506

Closed
wants to merge 13 commits into from
Closed

Chapter Indicator #506

wants to merge 13 commits into from

Conversation

Xliff
Copy link

@Xliff Xliff commented May 17, 2016

This implements changes to page indicator to allow for chapter length and delta display. There is a new configuration option that controls the new behavior.

I find this helpful in determining when there is a natural stopping point, while reading.

There are small optimizations that can be made, and I know the current reader is being phased out. If there are positive comments on this feature, I will move on and add these changes, there.

…d delta display.

  + There are small optimizations that can be made.
@kovidgoyal
Copy link
Owner

In order to make this accurate you have to handle the case where more than one chapter is present in a single HTML file. See how it is done in viewer/toc.py (that allows the ToC to mark the section being currently read in bold).

@Xliff
Copy link
Author

Xliff commented May 18, 2016

Thanks. Will do that.

@Xliff
Copy link
Author

Xliff commented May 19, 2016

Kovid,

Do you know of an ePub file that has multiple chapters in a single HTML file? I've searched the web for examples, and I haven't had much luck. I have identified the part in toc.py that I need to emulate, but I don't have anything to test it against. It seems like most ePub authoring software depends on chapters to have split files.

I have one other thing I can try in Sigil, and I'll see how that works.

Thanks!

@kovidgoyal
Copy link
Owner

Just open any epub file in the calibre book editor and right click all the individual files and select merge.

@Xliff
Copy link
Author

Xliff commented May 19, 2016

Thank you! That's good to know. I am now looking at how things break down. Having multiple chapters in one page complicates things.

@Xliff
Copy link
Author

Xliff commented May 19, 2016

Kovid,

The nearest I've been able to get to directly comparing the page value to the TOC positional entries is to use TOCItem.start_src_offset.

I then attempt to convert that into a fraction to compare against the page value by dividing it by the character count of the current page, and then multiplying that by the number of pages in the current page.

However it appears that TOCItem.start_src_offset is not quite matching up to what I am expecting. What does TOCItem.start_src_offset measure?

Thanks.

@kovidgoyal
Copy link
Owner

See anchor_positions() in indexing.coffee for how that value is
obtained.

@Xliff
Copy link
Author

Xliff commented May 20, 2016

Thanks. After looking at the CFI code, I think that is going in the wrong direction. Similarly, the code in toc.py uses the TOCItem to determine current chapter, but doesn't indicate position.

I've since come up with another alternative. This is giving me better results, but they are still slightly off. Here is the relevant code:

if hasattr(self, 'current_page'):
    page_anchors = []
    for idx_e in self.current_page.index_entries:
        if idx_e.start_anchor is not None:
            we = self.view.document.mainFrame().findFirstElement(
                "#%s" % idx_e.start_anchor)
                if we is not None:
                    g = we.geometry()
                    cs = self.view.document.mainFrame().contentsSize()
                    f = float(g.right()) / float(cs.width()) * \
                        self.iterator.pages[self.current_index]
                    page_anchors.append(f + sum(self.iterator.pages[:self.current_index]))
    self.page_anchors = page_anchors

(This excerpt is placed at the end of viewer.main.update_indexing_state(), for the time being)

What we are doing here is finding the element with the associated anchor. Locating its position in the page, then dividing that by the content size to determine its fractional position. We then turn that into a location in the current page, and then add all of the proceeding pages to come up with a final result.

The problem here is that the numbers are consistently coming out slightly lower than they should, but this is the closest result I have managed to obtain, so I am hoping I am on the right track.

I will continue to try and refine this, but if you have some insight, I would appreciate the help.

Thanks.

@Xliff
Copy link
Author

Xliff commented May 21, 2016

Oddly enough, when I replaced this:

page_anchors.append(f + sum(self.iterator.pages[:self.current_index]))

with this:

page_anchors.append(f + self.current_page.start_page)

...it works!

I will make a new commit to the PR, shortly.

@kovidgoyal
Copy link
Owner

That implementation seems rather inefficient. Since the viewer is already iterating over the anchors and fetching their geometry once, in anchor_positions() in indexing.coffee -- why do it again? Especially since computing geometry is a very expensive operation (it causes a re-layout if the render tree is dirty).

self.anchor_positions in documentview.py already contains a mapping of anchor names to positions -- use that. Note that in paged mode the first position is a column number not a x offset. So, in paged mode you can convert that into a fraction by dividing it by the total number of columns rather than the body width.

@Xliff
Copy link
Author

Xliff commented May 21, 2016

Well that explains it.

My first implementation tried using anchor_positions, but the values were always unexpectedly low. Now that I realize what anchor_positions actually measures, this makes more sense.

Your suggestion to use the total number of columns may be the missing piece I was looking for, however I have yet to find this information. I will keep searching.

Xliff added 2 commits May 21, 2016 01:07
- Next implementation will attempt to use anchor_positions from DocumentView
@kovidgoyal
Copy link
Owner

See page_dimensions in documentview.py

Note that you probably need page_width rather than col_width as number of columns is document.body.scrollWidth / page_width

…ather than

  recomputing the geometry. This should be more efficient.
@Xliff
Copy link
Author

Xliff commented May 21, 2016

Kovid,

Now implemented.

I'm sure there are still issues with this implementation, but I've been up all night working on this. I finally had the "Eureka!" moment sometime around 8 o'clock this morning and crashed this out. I've tested it on my end, and it seems to work as well, if not better than the previous implementation, so thank you for your suggestions.

Please let me know if you have other issues with the PR. At this point, I am sure I can work everything else out. I will make more tests over the weekend, just to stress the implementation. I will let you know how things work out once that is done.

@kovidgoyal
Copy link
Owner

You have to make the code mode dependant -- in paged mode you use scrollWidth and page_width in flow mode you have to use scrollHeight (maybe you already handle that -- wasn't clear from just looking at the last commit). Also you should store scrollWidth only at the end of after_load() not in update_contents_size_for_paged_mode() as in can change until then.

@Xliff
Copy link
Author

Xliff commented May 22, 2016

True enough. I haven't done that, yet. I wanted to get it working in paged
mode before working on the others. Thanks for the hints for flow mode.

On Sun, May 22, 2016 at 12:04 AM, Kovid Goyal notifications@github.com
wrote:

You have to make the code mode dependant -- in paged mode you use
scrollWidth and page_width in flow mode you have to use scrollHeight (maybe
you already handle that -- wasn't clear from just looking at the last
commit). Also you should store scrollWidth only at the end of after_load()
not in update_contents_size_for_paged_mode() as in can change until then.


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#506 (comment)

	TypeError: a dict value has type 'unicode' but 'QString' is expected
@@ -88,14 +88,16 @@ def readvar(name):
from PyQt5.QtCore import PYQT_CONFIGURATION
pyqt['sip_flags'] = PYQT_CONFIGURATION['sip_flags']
def get_sip_dir():
q = os.environ.get('SIP_DIR', sys.prefix if iswindows else os.path.join(sys.prefix, 'share', 'sip'))
Copy link
Contributor

Choose a reason for hiding this comment

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

this seems not right - are you sure about this change?

Copy link
Author

Choose a reason for hiding this comment

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

You are correct, that change was necessary to get the code working in my debug environment. It does not belong in the PR.

@@ -759,6 +787,19 @@ def update_indexing_state(self, anchor_positions=None):
self.toc_model.currently_viewed_entry, entry,
pgns[2])
self.toc_clicked(entry.index(), force=True)
#if hasattr(self, 'current_page'):
Copy link
Contributor

Choose a reason for hiding this comment

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

not a fan of having commented code - likely to be stale and can't be automatically be checked.

Copy link
Author

Choose a reason for hiding this comment

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

Again, this PR is still under development, and I didn't want to have to mess with git to revert changes.

@kovidgoyal
Copy link
Owner

This PR is likely going to require a complete re-write, sincethe viewer has be re-written for calibre 4 to use Qt WebEngine.

@kovidgoyal kovidgoyal closed this Oct 3, 2019
@Xliff
Copy link
Author

Xliff commented Oct 4, 2019

Have these changes been pushed to the master branch?

Also, when is Calibre 4 due to be released?

@kovidgoyal
Copy link
Owner

kovidgoyal commented Oct 4, 2019 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants