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

Don't hide the setting if no-update config was used #4138

Merged
merged 1 commit into from
May 14, 2020

Conversation

Krzmbrzl
Copy link
Member

@Krzmbrzl Krzmbrzl commented May 6, 2020

With commit 847ed7a it was introduced
that when using the no-update config flag, not only the default values
for checking or not checking for updates were affected but actually the
whole setting would be hidden from the UI.

This commit reverts that as a user should always be able to decide for
him- or herself whether or not they want to see update notifications or
not.

It does make sense for package maintainers to disable these checks by
default (as updates are received via package manager anyways) but if the
user wants to have that check enabled nonetheless, they should be able to
do so.

Context: Discussion in #4128

/cc @toby63

@toby63
Copy link
Contributor

toby63 commented May 6, 2020

I believe some will object to this, so I want to add some things.
The goal: I would like as many users as possible to use recent versions (many reasons for this, including security).
But if they are not informed about any updates, this cannot be achieved.
The compromise of the discussion (in #4128) was, to at least allow users to enable the update notifications by themselves.

Further notes (this applies to package users only):

  1. This is not urgent, it can be implemented once its ready.
  2. It could be placed in extended settings
  3. A notice could be added, that a package version is used that is automatically downloaded via package managers
  4. The message shown to the user could be modified:
    • "new version available: message your package maintainer"
    • "your distro might keep older packages, if you want to upgrade you have the following options:"
  5. Point 2, 3 & 4 could be "bound" to the option that package maintainers will use (CONFIG+=no-update).

Update: #4145 gave me another idea:
This would also be a very good compromise with package maintainers (and distros alike).
What if you would notice/inform the user about available backport repos (and maybe official mumble repos like PPAs)?

With commit 847ed7a it was introduced
that when using the no-update config flag, not only the default values
for checking or not checking for updates were affected but actually the
whole setting would be hidden from the UI.

This commit reverts that as a user should always be able to decide for
him- or herself whether or not they want to see update notifications or
not.

It does make sense for package maintainers to disable these checks by
default (as updates are received via package manager anyways) but if the
user wants to have that check enabled nonetheless, they should be able to
do so.
@Krzmbrzl Krzmbrzl merged commit 02863e5 into mumble-voip:master May 14, 2020
@toby63
Copy link
Contributor

toby63 commented May 14, 2020

Even though it was my idea, I have to object against implementing it this way.
It can be confusing for people (because some might think "hmmm 🤔 why is this not activated?")
If this is implemented, there should at least be:

  • a text next to the option clarifying that this disabled, because mumble is installed via package
  • (or/and) a hurdle (like put the option in "extended options" (I know there are none for now; they don't have to be hidden; it should just make clear that these are options for users who know what to do))
  • (optional, recommended) a different message (I don't know what the exact message is right now, but if it reads something like "go to our website to download the new client" this can be missleading for package users)

I know that this might leads to not implementing this at all, but I have to be fair.

@Krzmbrzl
Copy link
Member Author

Huh? If you wonder why it is disabled, you should simply enable it and off you go.

@toby63
Copy link
Contributor

toby63 commented May 14, 2020

Huh? If you wonder why it is disabled, you should simply enable it and off you go.

But a potencial missleading message is shown and it is not necessarily shown right now, but first a few weeks, months later.
I bet that this was the reason the former devs had disabled this alltogether.

@Krzmbrzl
Copy link
Member Author

What potential misleading message? The message states that there is a newer version available. That is the case. No misleading there.

And why a few weeks later?

@toby63
Copy link
Contributor

toby63 commented May 14, 2020

What potential misleading message? The message states that there is a newer version available. That is the case. No misleading there.

Ok, if thats the case everything is quite ok, though not ideal imo, but ok 🅱️ .

And why a few weeks later?

If the version is recent, then the message will only appear when the next version is available (thus weeks of months later).

@Krzmbrzl
Copy link
Member Author

Yeah of course the message will only be shown if there actually is a new version available.

I'm actually quite confused about your doubts here. As I understood it, you said that you think it's bad that the user can't chose to see version updates. That's possible now...

And I kinda dislike that you come out with this now that it is merged. There was no activity on this PR for over a week. If there is stuff that needs to be discussed, it's always better to do so while the PR is still open.
Don't get me wrong though. I think it's better to have the discussion after the merge than not having it at all, but in this specific case I think it would have been possible to bring these points up before the merge 🤷

@toby63
Copy link
Contributor

toby63 commented May 14, 2020

Yeah of course the message will only be shown if there actually is a new version available.

You seem to forget your own arguments in five minutes, it doesn't matter now, the point was:

  1. You said that a user would see the result of activating this option (right then):

you should simply enable it and off you go.

  1. This is not the case if there is no new version (right then).

I'm actually quite confused

Yeah I am also confused by you being confused.
Maybe one more time:

  1. The user sees this option and thinks ("why is this disabled? it should be enabled")
  2. So he enables it.
  3. Now a new version comes out, user is informed -> user is now confused, because he installed a package version
    -> thus my ideas of better clarifying what this all means (for a package user).

And I kinda dislike that you come out with this now that it is merged.

Take a look: #4138 (comment)
I commented long ago.

So no offence, this is complicated, but its not my fault, that its complicated 🙂.
(And also not yours of course).
But I admit, maybe I have led you in a wrong direction, but I already clarified that 8 days ago.

@Krzmbrzl
Copy link
Member Author

I think there's a misunderstanding here. First off your comment to this PR seemed like a defense against anyone that potentially dislikes the changes made. Thus I read it as "If you disagree with these changes being made, there night be a compromise by using the following options...".
Second I never said (or at least never meant to say) that enabling this option will immediately show an update notification. I said that you can turn update notifications on at every time, if you feel like it. The action actually does have "immediate effect" (right at the next startup) by checking for updates. However the actual update notification is of course only shown if the check actually finds an update.

This is how the message looked for the 1.3.0 release: https://mumble.info/focus.php
The message is actually downloaded like this from the remote update server so we can't change it to be different for package-users.

The only option that might make sense in my eyes would be to adapt the tooltip for that option to include a hint about the package situation.

And besides: As the option is disabled by default I expect the majority of users to leave it that way anyways. And the ones that explicitly do turn it on, should know enough about how updates work to also understand the package situation 🤷

@streaps
Copy link

streaps commented May 15, 2020

"And the ones that explicitly do turn it on, should know enough about how updates work to also understand the package situation"

But maybe they don't. That they should know enough about how updates work is only an opinion, but we don't know that for a fact.

I still think the expectation is that if there is a check for updates, the application can also be upgraded when there is a new version available. Getting an announcement for a new version and then be unable to upgrade it can be a frustrating experience.

The decision to remove the setting with NO_UPDATE_CHECK was maybe not the perfect solution for everyone, but I still think it's better than the potential confusions.

I also don't think that the behaviour of a build option should be changed. Now distributions would have to apply a custom patch to remove it.

@toby63
Copy link
Contributor

toby63 commented May 15, 2020

As much as I don't like it, I agree with @streaps, this change should be reverted for now.

I guess I will try to talk to some package maintainers about the general topic of outdated mumble versions and see if there are other solutions.

For example:

  • We could point users to backport repos
  • We could inform users later (so package maintainers have more time to supply new versions)
  • We could have information about newer versions in old clients or at least a hint that they might be outdated

I don't know if distros/maintainers will like that, but I guess we will see when I talk to them.

@Krzmbrzl
Copy link
Member Author

Actually this change made the option work as described again:

mumble/INSTALL

Lines 139 to 140 in 02863e5

CONFIG+=no-update (Mumble)
Default disable the checking of new versions. (For distributions)

@toby63
Copy link
Contributor

toby63 commented May 15, 2020

@Krzmbrzl Yes, but don't you think that all participants know how it worked until yesterday?
I am sure there was a reason the former devs implemented it this way.

The Alternative would be, to change this description:

mumble/INSTALL

Lines 139 to 140 in 02863e5

CONFIG+=no-update (Mumble)
Default disable the checking of new versions. (For distributions)

Or if "we" really want to offer free choice, implement a second option ("disable default" and "disable update option"), so the package maintainers would decide (not ideal imo).

Or you implement some of my suggestions, so that users understand the option and the message correctly.

@Krzmbrzl
Copy link
Member Author

Krzmbrzl commented May 15, 2020

Yes, but don't you think that all participants know how it worked until yesterday?

I see this similar to the cpp standard. Only what the standard really says can be relied on. Everything that you know a certain compiler does anyways is per se undefined behavior and if you make use of it, you can't complain if stuff breaks 🤷

Or you implement some of my suggestions, so that users understand the option and the message correctly.

As I said: We / I could change the tooltip or maybe even let a pop-up jump in your face if you try to enable it. Actually I think we should be good if we have the pop-up stating that found updates are probably not applicable for this specific kind of build...

@toby63
Copy link
Contributor

toby63 commented May 15, 2020

Only what the standard really says can be relied on.

Fair enough.

Actually I think we should be good if we have the pop-up stating that found updates are probably not applicable for this specific kind of build...

Yeah that could be an ok solution, some thoughts:

  • Like I said before you could combine the "no-update"-build-option and the activation of such a pop-up (so only package users see it) (you probably thought about that, but I wanted to mention it just in case).
  • Better formulation could be something like:
You are using a mumble package from distro repos, so normally no manual update is necessary. 
In case you are using an older distro version and want to use a newer version of mumble, take a look at: https://wiki.mumble.info/wiki/Installing_Mumble for potencial alternatives

Regarding Alternatives:

  • Different update message:

The message is actually downloaded like this from the remote update server so we can't change it to be different for package-users.

Well it can be changed (in future versions): the client would download a different message ;).

  • a different solution, like a "later-update-notification" would still be possible.
    I ask some package maintainers what they think about such an idea.

@Krzmbrzl
Copy link
Member Author

you probably thought about that, but I wanted to mention it just in case

Exactly :)

Well it can be changed (in future versions): the client would download a different message ;).

Yes but that'd add another item on the things that must not be forgotten when drafting a new release. I'd like to not add to that xD

I ask some package maintainers what they think about such an idea.

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants