Timeline that displays PDF files

[FelipeDefensor]

@azfoo, could you take a look at this? I think you're familiar enough with the codebase that you might be able to spot something I have missed. Don't worry if you can't understand all of it.

[FelipeDefensor]

Nice work!

Here are a few other things that might need looking into:

1. The timeline continuously increments page numbers from the last highest page number input. This results in the page numbers increasing beyond the last page of the pdf. There isn't any error thrown, but the viewer remains on the first page. Should this be an error instead of having it happen silently?

2. Changing height on this timeline doesn't really do anything. The icons get cropped if the height is changed to less than the default 30 and becomes slightly unclickable. I guess the other question would be if there there is a need for the user to change height on this timeline in the first place .

3. Attempting to add this timeline without any media causes a crash.

4. On the side facing the user, I think pdf should be either standardised as pdf or PDF, instead of having Pdf thrown in the mix too.

Thanks for the review!

1. Nice catch. I will cap the auto-generated page numbers to the last page. I had trouble preventing the user from setting an out of bounds value in the inspector and I just resigned to PyQt default behavior, which is to show the first page. I just had an idea that might solve that, though.
2. As you figured, there isn't a point in changing this timeline height, which I why I didn't bother implementing. I did forget, however, to remove the menu option for the user to do so.
3. Will fix.
4. Absolutely. Let's go with PDF.

[FelipeDefensor]

@azfoo fixed what you mentioned, added a bunch of things I forgot and also fixed some bugs that revealed themselves in the process. I will squash everything related to the PDF timeline into a single commit before merging.

[azfoo]

I don't know if the page number on the UI is supposed to be an index or the actual page number. It shows up as the right number on the inspector, but not the UI. Pressing enter to open the inspector on the elements with the wrong number causes the spinbox to update the UI to show the right value.

I'm also wondering if being able to change the source of the PDF would be a necessary function.

[FelipeDefensor]

I don't know if the page number on the UI is supposed to be an index or the actual page number. It shows up as the right number on the inspector, but not the UI. Pressing enter to open the inspector on the elements with the wrong number causes the spinbox to update the UI to show the right value.

It's supposed to be the actual page number. I tested it again here, and it seems to work. See this:

https://github.com/TimeLineAnnotator/desktop/assets/87732202/21a7a9b0-0d59-4571-845b-722e0d5911fb

[FelipeDefensor]

I'm also wondering if being able to change the source of the PDF would be a necessary function.

I thought about that and coudn't think of any use case that wouldn't be solved just as well by creating a new timeline. Changing the source PDF on an existing timeline would only be useful if you wanted to preserve the existing pdf markers. But, if you're changing the file, how could the markers still indicate meaningful timestamps on that new file? Even in that improbable case, the user can still copy the markes over to the new timeline.

It wouldn't be too much trouble implementing that functionality though, with what we already have. Do you think it's worth it?

[azfoo]

I don't know if the page number on the UI is supposed to be an index or the actual page number. It shows up as the right number on the inspector, but not the UI. Pressing enter to open the inspector on the elements with the wrong number causes the spinbox to update the UI to show the right value.

It's supposed to be the actual page number. I tested it again here, and it seems to work. See this:

If you add a marker, and set it to the last page, then add another marker at a time after that, the UI still shows the new marker to be a higher page than the last possible page.

[azfoo]

I'm also wondering if being able to change the source of the PDF would be a necessary function.

It wouldn't be too much trouble implementing that functionality though, with what we already have. Do you think it's worth it?

Good point. I guess it isn't necessary then.

[azfoo]

Two more points:

• I don't know if the delete button is necessary, considering all other components can be ordinarily deleted with delete on the keyboard.
• While it isn't possible to close the viewer from the window itself, closing the viewer from the OS makes it impossible to reopen without restarting the app. Should there be a different way to reopen the window?

[FelipeDefensor]

Two more points:

* I don't know if the delete button is necessary, considering all other components can be ordinarily deleted with delete on the keyboard.

Good point. We should also remove the analogous button on the marker timeline.

* While it isn't possible to close the viewer from the window itself, closing the viewer from the OS makes it impossible to reopen without restarting the app. Should there be a different way to reopen the window?

Yes, I think there should. We have an analogous problem with the video window. Maybe it's time to create a "View" menu where the user can toggle the visibility of those windows. I have created a task in the project with High priority for this.

[FelipeDefensor]

I don't know if the page number on the UI is supposed to be an index or the actual page number. It shows up as the right number on the inspector, but not the UI. Pressing enter to open the inspector on the elements with the wrong number causes the spinbox to update the UI to show the right value.

It's supposed to be the actual page number. I tested it again here, and it seems to work. See this:

If you add a marker, and set it to the last page, then add another marker at a time after that, the UI still shows the new marker to be a higher page than the last possible page.

That would be the kind of thing that would get me very frustrated once I found it weeks later. Glad I've got another pair of eyes to spot it.

[FelipeDefensor]

Well, I think that's all of it, for now.

[FelipeDefensor]

Could you also remove the deletes from the list in TiliaAction?

Actually, no, they are used by the right-click menus.

And I'm not sure if its a me thing again, but the pdf tests are failing.

Not a you thing at all. Fixed them.

[azfoo]

Yay! I think that's all done then.