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

Warn about apps built against EOL runtimes #1804

Closed
wants to merge 12 commits into from

Conversation

meisenzahl
Copy link
Member

@meisenzahl meisenzahl commented Jan 7, 2022

Re: #1803

Peek.2022-01-07.12-05.mp4

@meisenzahl meisenzahl requested a review from a team January 7, 2022 11:07
src/Views/AppInfoView.vala Outdated Show resolved Hide resolved
Copy link
Contributor

@cassidyjames cassidyjames left a comment

Choose a reason for hiding this comment

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

An initial pass; feel free to push back on any changes! But here's what I'm thinking:

  • Flatpak seems to use eol as the end of life terminology consistently rather than deprecated, so I think it makes sense to align our code to that for clarity.
  • However, End of Life might not really mean what users expect, especially when we're talking about the runtimes and not necessarily the app itself (unless I'm reading this wrong, in which case, oops!).
  • So, I made the user-facing copy more generic; this also reflects somewhat how iOS and Android talk about apps made for an older version of the OS, but we can't really say "for an older version of elementary OS" because that's not exactly how runtimes work.
  • Lastly, I don't see us needing to care about the number of EOL runtimes, so I refactored that into a bool. Edit: I see you started with an is_eol bool so this was basically just reverting back to that while using a more precise name; feel free to change this further if it can be simplified.

src/Core/FlatpakBackend.vala Outdated Show resolved Hide resolved
src/Core/FlatpakBackend.vala Outdated Show resolved Hide resolved
src/Core/FlatpakBackend.vala Outdated Show resolved Hide resolved
src/Core/Package.vala Outdated Show resolved Hide resolved
src/Views/AppInfoView.vala Outdated Show resolved Hide resolved
src/Views/AppInfoView.vala Outdated Show resolved Hide resolved
src/Views/AppInfoView.vala Outdated Show resolved Hide resolved
@cassidyjames cassidyjames changed the title Warn about apps built against old runtimes Warn about apps built against EOL runtimes Jan 7, 2022
@cassidyjames
Copy link
Contributor

I also want to note that this doesn't precisely close #1803; that issue is about a mismatch between the OS version and the runtime version for curated apps, while this PR focuses on EOL runtimes. I think both are valuable, but what @danrabbit is asking for is a little different; if I install an app build against the not-EOL OS 6 runtime on OS 7, it might be useful to still point out that the app was built for OS 6 and thus might be missing out on features/integration.

In that situation, I think EOL warning would take precedence, but we'd need to do some version matching and only look at curated apps, etc. I think that is a separate PR, though.

@danirabbit
Copy link
Member

Yeah for EOL I'd be inclined to use a much larger warning like a floating infobar

@cassidyjames
Copy link
Contributor

@danrabbit I think I'm still fine with using our content warning space for EOL runtimes, as it keeps the security/updates/expectations information in one place, but I could see that argument, too. 🤔

Co-authored-by: Cassidy James Blaede <[email protected]>
Copy link
Contributor

@cassidyjames cassidyjames left a comment

Choose a reason for hiding this comment

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

Alright, this reverts back further to basically where your is_eol bool was, but using more precise terminology. Does that make sense?

src/Core/FlatpakBackend.vala Outdated Show resolved Hide resolved
src/Core/FlatpakBackend.vala Outdated Show resolved Hide resolved
src/Core/FlatpakBackend.vala Outdated Show resolved Hide resolved
@meisenzahl
Copy link
Member Author

I also want to note that this doesn't precisely close #1803; that issue is about a mismatch between the OS version and the runtime version for curated apps, while this PR focuses on EOL runtimes. I think both are valuable, but what @danrabbit is asking for is a little different; if I install an app build against the not-EOL OS 6 runtime on OS 7, it might be useful to still point out that the app was built for OS 6 and thus might be missing out on features/integration.

I must have missed something in the issue 😅

But that shouldn't be a problem to implement. I am preparing a branch based on this PR.

Co-authored-by: Cassidy James Blaede <[email protected]>
@cassidyjames cassidyjames requested a review from a team January 7, 2022 18:10
Copy link
Contributor

@cassidyjames cassidyjames left a comment

Choose a reason for hiding this comment

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

I'm happy with this as an initial pass UX wise. It adds value and uses existing mechanisms to present an important bit of information. I think I'd still like a pass from @elementary/desktop-developers in case I missed anything code-wise, though!

Screenshot from 2022-01-07 11-11-16

Copy link
Contributor

@cassidyjames cassidyjames left a comment

Choose a reason for hiding this comment

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

oops ignore this accidental double review

@danirabbit danirabbit closed this Jan 11, 2022
@danirabbit danirabbit deleted the warn-about-apps-built-against-old-runtimes branch May 14, 2022 19:16
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

3 participants