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

Replace deprecated setTimeSpec() calls #779

Merged

Conversation

TheComputerGuy96
Copy link
Contributor

@TheComputerGuy96 TheComputerGuy96 commented Oct 19, 2024

No description provided.

@kevinhendricks
Copy link
Contributor

kevinhendricks commented Oct 19, 2024

Unfortunately, the correct solution for the "unused variable" is to properly use it at the end of that routine. That said, the GetInfo routine is a new addition yesterday and is testable but not complete.

I have pushed the proper solution to master.

Please remove your incorrect unused variable "fix", and I will go ahead and merge the remaining part of your PR.

Thank you.

@kevinhendricks
Copy link
Contributor

Actually since Sigil supports as far back as Qt 6.4.2 and QTimeZone did not finish that class until Qt 6.5, and because setTimeSpec will not even be deprecated until 6.9, I am not sure we can merge your PR at all. It would need to be tested on Qt 6.4 to see if it works properly.

@dougmassay
Copy link
Contributor

I approved the Linux CI build of this PR, so we should at least see if it will build with Qt6.4

@kevinhendricks
Copy link
Contributor

So it appears to work with Qt 6.4.
So we can merge this once amended to remove the improper fix for the "unused variable".

@dougmassay
Copy link
Contributor

It builds ok, but I'm not sure that's enough. It wouldn't be the first time I saw a Qt problem that didn't manifest until runtime. ;) How would I test that this works with a Qt6.4 based Sigil?

That said: we can probably merge the PR for now. If we run into problems later, we can deal with it.

@kevinhendricks
Copy link
Contributor

But we only want 7eba71a

We do not want 057540b, as that variable is no longer unsed.

@dougmassay
Copy link
Contributor

Got it. So we're still waiting for PR to be changed to reflect that.

@kevinhendricks
Copy link
Contributor

Yes, unless you know how to merge only a part of a PR!

@dougmassay
Copy link
Contributor

dougmassay commented Oct 19, 2024 via email

@kevinhendricks
Copy link
Contributor

kevinhendricks commented Oct 19, 2024

You can test it by editing any epub and saving it and then check the metadata for modification time to see if it fits the required format (matches the current format) and has the current date/time.

@kevinhendricks
Copy link
Contributor

Let's give them a chance to fix the PR, otherwise we will cherry pick the part we want.

@dougmassay
Copy link
Contributor

Looks like the PR might be incompatible with the current HEAD. Is that just because of the commit in question?

@kevinhendricks
Copy link
Contributor

kevinhendricks commented Oct 19, 2024

Yes, it removed an "unused" variable in the newly created GetInfo() but instead of removing it, it should have been used to restore BookBrowser state.

This change should be compatible back to Qt 5.5 (https://doc.qt.io/qt-5/qtimezone.html#utc)
and it removes the deprecation warnings that occur since Qt 6.5
@TheComputerGuy96
Copy link
Contributor Author

I was actually sleeping during this time (anyway I removed the unused variable change)

@kevinhendricks kevinhendricks merged commit 2b9cf3c into Sigil-Ebook:master Oct 20, 2024
4 checks passed
@kevinhendricks
Copy link
Contributor

Thank you!

kevinhendricks added a commit that referenced this pull request Oct 25, 2024
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.

3 participants