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

feat: check for updates #7

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

feat: check for updates #7

wants to merge 3 commits into from

Conversation

r3tr0ananas
Copy link
Member

image

@r3tr0ananas r3tr0ananas linked an issue Jan 15, 2025 that may be closed by this pull request
3 tasks
@THEGOLDENPRO
Copy link
Member

Good idea but I think this will be better off outside of the egui crate as a standalone crate like theming and also having notification toast and other egui things be handled by the application itself other than the crate. You can name the crate something like updates.

Crossed out that line above because I though you were handling toasts in the crate based on the egui-notify dependency but instead you were just using it for egui_notify::ToastLevel. Well that's not needed and I see you are returning errors as Some(). It would be better to remove that and then return a Result<T, E> instead.

Also I feel like there should be an individual method (get_latest_version()) that goes and gets the latest version number from github and just returns that with possibly the type Result<Option<semver::Version>, E>, as if you get an error return Err(), if there's just no release available to return a version number then return Ok(None) and for version numbers return the semver::Version wrapped in Ok(Some()).

Now if you want you can keep check() but instead I advise having it return a Result. Now the toast messages, I'm not 100% sure if they should just be moved to the application for now and in the future I'll figure something out for UI consistency like a widget applications can use from the egui crate or what.

But I hope you understand where I'm getting to; I think the update stuff should be completely split away from egui like the theming crate so we can potentially make use of it in other UI libraries and projects. Leave UI stuff to the egui crate and everything else that doesn't have to be egui only outside of the egui crate if possible.

Also btw, you don't have to do this. You can leave this to me and my spare time.

@THEGOLDENPRO
Copy link
Member

Also sorry for late response, was busy and I didn't have this repo set on watch.

@r3tr0ananas
Copy link
Member Author

Also btw, you don't have to do this. You can leave this to me and my spare time.

I can do it, i'm not busy rn.

@THEGOLDENPRO
Copy link
Member

THEGOLDENPRO commented Jan 17, 2025

just dumping a note here for me to see and remember on sunday hopefully:

  • sccache
  • cirrus dep changes
  • error handling

@THEGOLDENPRO
Copy link
Member

Sorry for this pr taking longer than usual. For now you can just slide this code directly into aeternum then in the future when this is merged into main you can switch to the cirrus implementation.

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.

None yet

2 participants