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

RELEASE_NOTES for 4.2: Gtk 3.24.15 min needed #13169

Merged
merged 3 commits into from
Dec 20, 2022

Conversation

Nilvus
Copy link
Contributor

@Nilvus Nilvus commented Dec 19, 2022

Closes #13166.

Thanks to @jade-nl issue, found that new font-feature-setting: "tnum"; added is only supported since march of 2020, with Gtk+3 3.24.15. I think it remains good but actually darktable specs require only 3.22. That seems to remains good except for that new line. As 3.24.15 is now quite old, I think we could keep it as it but that means that darktable will not work correctly by default on older release.

What do we do for that? For 4.2 release, I propose to update release notes as suggested by that PR. But that's probably not enough. Or we report that line and remove it by now (it's useful but we could deal without), or we propose as a minimum 3.24.15 instead of 3.22.

@dterrahe: as that line proposal is from you, your feedback is welcomed.

@Nilvus
Copy link
Contributor Author

Nilvus commented Dec 19, 2022

@TurboGit: as suggested by @parafin on linked issue there, remains cmake and doc (@elstoc). Sorry for the mess and 3 commits for such thing here. I do all that directly on Github and forget some things on my way... Feel free to commit all that on one commit if needed and squash them all.

@Nilvus Nilvus added this to the 4.2 milestone Dec 19, 2022
@Nilvus Nilvus added documentation-pending a documentation work is required release notes: pending labels Dec 19, 2022
@Nilvus Nilvus requested a review from TurboGit December 19, 2022 22:00
@parafin
Copy link
Member

parafin commented Dec 19, 2022

By doc I mostly meant README.md, I’m not sure where else we mention dependencies.

@elstoc
Copy link
Contributor

elstoc commented Dec 19, 2022

Obviously it's too late to change the release (though we could push a quick 4.2.1 after) and it does seem a shame to insist on a later Gtk version for a one-line change (unless it fixes a major issue). If you plan to change the release notes, it will need rewording. Perhaps something like:

In order to support the correct display of numbers in darktable, the minimum supported Gtk version has had to be increased to 3.24.15. For people who need to build darktable with an older version, this can be supported by reverting the following change: ...

It would be good if we could be very explicit about the issue that it fixes (I'm not sure "the correct display of numbers" is enough info) and give a link to the commit that changed it (so that people can see what changed and either manually remove it or revert the commit locally).

Alternatively, if we think that is a reasonable minimum version to support, I suggest we forego the description entirely and just state the revised dependency.

@dterrahe
Copy link
Member

Alternatively, if we think that is a reasonable minimum version to support, I suggest we forego the description entirely and just state the revised dependency.

We've regularly seen recent "maintenance" updates to the legacy gtk 3 series introducing regressions that required workarounds but we would only see/apply these if the dev runs those versions (or if we get a very clear bug report and the reporter helps with a workaround rather than just upgrading gtk themselves). So I would be surprised if there were not a slew of other bugs with older versions of gtk, that might cause subtle or not-so-subtle but not disastrous to the point of unworkable bad, and therefore unreported, behavior. Pointing out one such issue somewhat suggests that other than that older versions of gtk are "supported" when they are not. I'd just update the dependency...

@Nilvus
Copy link
Contributor Author

Nilvus commented Dec 19, 2022

I just changed release notes as I think it would be good to precise that here and/or for website when we post 4.2 release on wednesday, outside packages. I let you @TurboGit decide what to do with that and update my text by @elstoc proposal. If needed, close that PR and update what's possible where it's possible. It's just to help and deal with linked issue. I hope we will have a few people with so older release anyway.

And I agree @elstoc that it's a shame to insist on that for such minor change but that minor change with bad Gtk release make a huge impact. About commit proposal to revert, not a good thing because that commit include another fix and reverting it would break another thing (minor one). Commit is that one: 7b3d15b

@Nilvus
Copy link
Contributor Author

Nilvus commented Dec 19, 2022

And so, I also agree to just bump Gtk minimal dependency and remove any description about the issue.

@elstoc
Copy link
Contributor

elstoc commented Dec 19, 2022

About commit proposal to revert, not a good thing because that commit include another fix and reverting it would break another thing (minor one)

Ok fair point. In retrospect, splitting that change into two commits would have been better than trying to fix two things at once, just to make our lives easier if we did have to revert something. Commits are free

@Nilvus
Copy link
Contributor Author

Nilvus commented Dec 19, 2022

About commit proposal to revert, not a good thing because that commit include another fix and reverting it would break another thing (minor one)

Ok fair point. In retrospect, splitting that change into two commits would have been better than trying to fix two things at once, just to make our lives easier if we did have to revert something. Commits are free

You're right but for such minor CSS adds, thinking that a revert could had been needed like what we find now, was impossible at that time.

@elstoc
Copy link
Contributor

elstoc commented Dec 19, 2022

thinking that a revert could had been needed like what we find now, was impossible at that time

Completely agree. It's never possible to know what unintended side-effects a change might have, but small commits that do a single thing are generally just a good habit to get into, regardless of the change you're making.

@TurboGit
Copy link
Member

I think the current proposal is the best for 4.2 release. The implication is really bad have having a note about what is wrong and how to fix that by simply removing a CSS line seems ok to me.

@Nilvus: I'll merge that now and will copy/paste the text in the release notes for GitHub release and dtorg announce.

@TurboGit TurboGit merged commit f52909a into darktable-org:master Dec 20, 2022
@elstoc
Copy link
Contributor

elstoc commented Dec 20, 2022

was there something wrong with the wording I suggested? The current wording is really quite hard to follow

@TurboGit
Copy link
Member

was there something wrong with the wording I suggested? The current wording is really quite hard to follow

Yes, I missed it :) Now changed, thanks @elstoc !

@Nilvus Nilvus deleted the patch-3 branch December 20, 2022 18:22
@elstoc elstoc removed the documentation-pending a documentation work is required label Dec 22, 2022
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.

half/non-working UI (workaround available)
5 participants