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

updatechecker: Fix logic comparing version numbers #1284

Merged
merged 1 commit into from
Oct 17, 2023

Conversation

techee
Copy link
Member

@techee techee commented Oct 17, 2023

The condition is wrong and for version 2.0 it will constantly report there's an update available when upgrading from 1.x (where x > 0).

I had a "visionary" moment regarding possible 2.0 problems and had a look at the updatechecker plugin if it handles major versions - id does, but wrongly (in addition, there are two identical conditions for the "minor" version but the "mini" version check is missing).

I haven't tested this at all (not sure how to test when there's no actual release).

@b4n @eht16 I think this or some other variant of this patch should go to 2.0.

The condition is wrong and for version 2.0 it will constantly report
there's an update available when upgrading from 1.x (where x > 0).
@techee techee added this to the 1.39.0/2.0.0 milestone Oct 17, 2023
Copy link
Member

@b4n b4n left a comment

Choose a reason for hiding this comment

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

LGTM (not tested)

Indeed the check is wrong in many aspects -- event 0.0.2 would be < then 1.38.1
Also, it uses GEANY_VERSION, which is a compile-time thing, so better update your update checker! It could benefit from Geany exposing main_get_version_string() or similar (but probably not under that name).

@techee
Copy link
Member Author

techee commented Oct 17, 2023

Indeed the check is wrong in many aspects -- event 0.0.2 would be < then 1.38.1

Here you'll be saved by the copy-paste error of the current code where the last digit is ignored ;-).

The current code won't behave as bad as I thought originally because for releases the value on the server is the highest number. It would be just in the meantime, like now, where we already have 2.0 and server says 1.38 and for the code 1.38 > 2.0. Anyway, better to fix and also handle the last digit of the version.

Also, it uses GEANY_VERSION, which is a compile-time thing, so better update your update checker! It could benefit from Geany exposing main_get_version_string() or similar (but probably not under that name).

Yeah, but hopefully most of the time users update geany+plugins at the same time.

@techee techee merged commit e794ccc into geany:master Oct 17, 2023
2 checks passed
@b4n
Copy link
Member

b4n commented Oct 17, 2023

Here you'll be saved by the copy-paste error of the current code where the last digit is ignored ;-).

Ah indeed, missed that typo :)

The current code won't behave as bad as I thought originally because for releases the value on the server is the highest number. It would be just in the meantime, like now, where we already have 2.0 and server says 1.38 and for the code 1.38 > 2.0.

Well, one issue is that people using the plugin won't be notified about 2.0 anyway, as it'll make it older than basically whatever their version is… but we can't do anything about that.

@techee
Copy link
Member Author

techee commented Oct 17, 2023

Well, one issue is that people using the plugin won't be notified about 2.0 anyway, as it'll make it older than basically whatever their version is… but we can't do anything about that.

They should get notified because of

geany_running.major < geany_current.major

The problem was rather the potential "false positive" update notifications of the plugin. (I think the biggest problem of the code was to reason about what actually it would do.)

@b4n
Copy link
Member

b4n commented Oct 17, 2023

Ah right, it was notifying when either major or minor was higher. So yeah, only false positives if the local version is newer than the release.

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