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

Add link to website if a new version is available #1980

Merged
merged 7 commits into from
Sep 3, 2021

Conversation

ann0see
Copy link
Member

@ann0see ann0see commented Aug 27, 2021

Short description of changes

Adds a clickable link in the "new jamulus version available" message:
update message server
update message client

I think it would be good to have this one in the next release.

Context: Fixes an issue?

Sometimes users might find it confusing to only get notified that a new version is available but no actionable "solution" is provided. Therefore this will add a link to a page which is still to be created (currently just a redirect to jamulus.io).

Also see: #370 (comment)

Does this change need documentation? What needs to be documented and how?

  • Screenshots needed???
  • CHANGELOG: Link download page in "update available" message to make updating easier

Status of this Pull Request

Working implementation.

What is missing until this pull request can be merged?
Two approvals.

Checklist

  • I've verified that this Pull Request follows the general code principles
  • I tested my code and it does what I want
  • My code follows the style guide
  • I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
  • I've filled all the content above

@ann0see ann0see force-pushed the feature/link-download-page branch from 20d576c to c4c4bf6 Compare August 27, 2021 19:07
@ann0see ann0see force-pushed the feature/link-download-page branch from c4c4bf6 to bfeed76 Compare August 27, 2021 19:16
@pljones
Copy link
Collaborator

pljones commented Aug 27, 2021

I'd prefer wording like "A Jamulus upgrade is available: click for details". Definitely without the exclamation mark (implies being commanded to something).

@ann0see
Copy link
Member Author

ann0see commented Aug 27, 2021

Yes. Can change that.

However the question is how the "click for details" page should look like. But that's something I need to worry about on the webpage.

@ann0see ann0see force-pushed the feature/link-download-page branch from bfeed76 to d112825 Compare August 27, 2021 19:27
@ann0see
Copy link
Member Author

ann0see commented Aug 27, 2021

Ok. Changed it. Need to wait until autobuild is finished.

@ann0see
Copy link
Member Author

ann0see commented Aug 27, 2021

Windows autobuild failed. Hopefully an update of the ASIO sdk will fix it: ann0see@2583800

Ok. Download the artifacts here: https://github.com/ann0see/jamulus/releases/tag/r_link-download-page
(with faked version 3.0.0) to show the update message.

@ann0see
Copy link
Member Author

ann0see commented Aug 27, 2021

Ok. See the updated screenshot above.
Edit: fixed some spacing issues- hopefully. They weren’t included in the screenshot yet, but adding   should be trivial.

@ann0see
Copy link
Member Author

ann0see commented Aug 27, 2021

@gilgongo I think that‘s something for you?

@ann0see ann0see force-pushed the feature/link-download-page branch from d112825 to 6d0b21b Compare August 27, 2021 20:59
lblUpdateCheck->setText ( "<font color=\"red\"><b>" + QString ( APP_NAME ) + " " + tr ( "software upgrade available" ) + "</b></font>" );
lblUpdateCheck->setOpenExternalLinks ( true ); // enables opening a web browser if one clicks on a html link
lblUpdateCheck->setText (
"<font color=\"#c94a55\"><b>" + tr ( "A %1 upgrade is available:&nbsp;" ).arg ( APP_NAME ) +
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is the font colour changing?

Copy link
Member Author

Choose a reason for hiding this comment

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

The "red" didn’t fit to the mixer background contrast wise. But we can also agree on another colour.

Copy link
Collaborator

@pljones pljones Aug 29, 2021

Choose a reason for hiding this comment

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

Ideally we'd have a stylesheet (managed at application level) with a class... But if it's got to be an explicit hex code to look right, fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah. But that's out of scope here probably. I think you should open an issue (and maybe even tag it as "good first issue" since I think it's easy to implement)

src/serverdlg.cpp Outdated Show resolved Hide resolved
@pljones pljones added this to the Release 3.8.1 milestone Aug 29, 2021
@ann0see ann0see force-pushed the feature/link-download-page branch 2 times, most recently from 795626f to 9a16ea2 Compare August 31, 2021 18:56
@ann0see ann0see requested review from gilgongo and pljones August 31, 2021 18:57
Copy link
Member

@gilgongo gilgongo left a comment

Choose a reason for hiding this comment

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

Looks good.

@ann0see
Copy link
Member Author

ann0see commented Aug 31, 2021

Do we want to add a build version (with JACK, OS,...) as GET argument to the URL? This would enable us to automatically redirect the user to the correct download on jamulus.io

e.g.:

https://jamulus.io/upgrade?progversion=3.8.1&build=windows_jack while build= could also be a md5 hash of CONFIG QSysInfo::productType()

@ann0see ann0see force-pushed the feature/link-download-page branch from 9a16ea2 to 2ccecd2 Compare September 1, 2021 10:56
@ann0see
Copy link
Member Author

ann0see commented Sep 1, 2021

Used the wrong argument for the version on the update url. Should be fixed now.

src/serverdlg.cpp Outdated Show resolved Hide resolved
@ann0see ann0see force-pushed the feature/link-download-page branch 2 times, most recently from d92919a to 6a95ba4 Compare September 1, 2021 21:40
src/clientdlg.cpp Outdated Show resolved Hide resolved
@pljones
Copy link
Collaborator

pljones commented Sep 2, 2021

For testing, build locally with Jamulus.pro VERSION = 3.7.99 and check you get told 3.8.0 is available.

src/global.h Outdated Show resolved Hide resolved
@ann0see ann0see force-pushed the feature/link-download-page branch from 6a95ba4 to 89a201a Compare September 2, 2021 13:58
@ann0see
Copy link
Member Author

ann0see commented Sep 2, 2021

Before we merge this 72e4ac1 needs to be reverted. However, now we have it as test commit

ignotus666 and others added 2 commits September 2, 2021 18:04
* Update clientdlg.cpp

* Update serverdlg.cpp

* Update global.h

* Spacing
@pljones
Copy link
Collaborator

pljones commented Sep 2, 2021

So grab the artifact from the builds, run it as client and server, and let us know.

@ann0see
Copy link
Member Author

ann0see commented Sep 2, 2021

Yup. Works as expected.

@ann0see
Copy link
Member Author

ann0see commented Sep 2, 2021

Should be squash & merged.

@ann0see
Copy link
Member Author

ann0see commented Sep 2, 2021

I'm still not 100% sure about the color/contrast of the message. I'll revert it to red but maybe a @jamulussoftware/designers might give a proposal (preferably in another PR)

@mulyaj
Copy link
Contributor

mulyaj commented Sep 2, 2021

I just took a look here, and the current contrast ratio is 4.01:1.

Ideally, to be pass WCAG AA guidelines for normal text (~18px) you'd want a ratio of >4.5:1. I'd advise against a full tinted red, as I think it'd make the ratio worse.

A darker red (or something in this range) like #AB202C can work.

@ann0see
Copy link
Member Author

ann0see commented Sep 2, 2021

Yes. Looks much better now! Thanks

@mulyaj
Copy link
Contributor

mulyaj commented Sep 2, 2021

Oh, I didn't consider the color for the different skins.

I don't think the dark red I suggested works with the fancy skin. Are you open to using different colors for normal and fancy? Or would you prefer a color that encompasses both skins?

@pljones
Copy link
Collaborator

pljones commented Sep 2, 2021

Oh, I didn't consider the color for the different skins.

I don't think the dark red I suggested works with the fancy skin. Are you open to using different colors for normal and fancy? Or would you prefer a color that encompasses both skins?

I was going to ask.... Here it's not skinned. It was partly why I was thinking the styling should be passed as an argument (though I'd pulled all the HTML out), to make it easier to apply skinning.

@ann0see
Copy link
Member Author

ann0see commented Sep 3, 2021

It’s not that bad IMHO, but I think it’s out of scope of this PR to make the colours changable (since this would also involve the mute myself bar).

@pljones
Copy link
Collaborator

pljones commented Sep 3, 2021

It’s not that bad IMHO, but I think it’s out of scope of this PR to make the colours changable (since this would also involve the mute myself bar).

The server recording highlight is on the mixer panel, so I guess that's elsewhere, but that's skin-dependent, IIRC.

Stick with whatever colour is used in the Mute Myself indicator so at least we know it's meant to be the same in future.

@ann0see
Copy link
Member Author

ann0see commented Sep 3, 2021

Ok. Reverted it back to "red". See updated screenshots.

@pljones pljones merged commit f9ed108 into jamulussoftware:master Sep 3, 2021
@ann0see ann0see deleted the feature/link-download-page branch September 3, 2021 19:08
@ann0see
Copy link
Member Author

ann0see commented Sep 3, 2021

Thanks! I think this should have been squashed 😉

@pljones
Copy link
Collaborator

pljones commented Sep 4, 2021

Mmmm.... 😁

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.

5 participants