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.

Sign up for GitHub

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

Jump to bottom

Fix #2243 #2245

Merged
merged 2 commits into from
Aug 4, 2020
Merged

Fix #2243 #2245

merged 2 commits into from
Aug 4, 2020

Conversation

camilasan
Copy link
Member

@camilasan camilasan commented Aug 3, 2020 •
edited
Loading

@camilasan camilasan requested a review from er-vin August 3, 2020 19:40
@camilasan camilasan changed the title Fix for #2243 Fix #2243 Aug 3, 2020
@camilasan camilasan linked an issue Aug 3, 2020 that may be closed by this pull request
URL of default Talk button is wrong. #2243
Closed
er-vin
er-vin requested changes Aug 4, 2020
View reviewed changes
src/gui/tray/UserModel.cpp Outdated
@@ -486,6 +486,11 @@ bool User::serverHasTalk() const
return _account->hasTalk();
}

AccountApp *User::talkApp() const
{
return _account->findApp(QLatin1String("spreed"));
Copy link
Member

Choose a reason for hiding this comment

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

You're merely moving code around here, but good opportunity to use QStringLiteral instead of QLatin1String here.

src/gui/tray/UserModel.cpp Outdated
if(talkApp){
QDesktopServices::openUrl(talkApp->url());
} else {
qCInfo(lcActivity) << "The Talk app was not found on the server.";
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: I'd go for warning instead of info. It's not really supposed to happen, if we got in that else something was wrong in the caller.

src/gui/tray/UserModel.cpp Outdated
if (!(url.contains("http://") || url.contains("https://"))) {
url = "https://" + _users[_currentUserId]->server(false) + "/apps/spreed";
auto talkApp = _users[_currentUserId]->talkApp();
if(talkApp){
Copy link
Member

Choose a reason for hiding this comment

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

Missing space after if and before opening curly brace, should be: if (talkApp) {

src/gui/tray/UserModel.cpp Outdated
@@ -655,7 +647,8 @@ Q_INVOKABLE void UserModel::openCurrentAccountTalk()
if(talkApp){
QDesktopServices::openUrl(talkApp->url());
} else {
qCInfo(lcActivity) << "The Talk app was not found on the server.";
qCInfo(lcActivity) << "The Talk app is not enabled on " <<
_users[_currentUserId]->server();
Copy link
Member

Choose a reason for hiding this comment

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

I'd say use currentUser() instead of _users[_currentUserId].

src/gui/tray/UserModel.cpp Outdated
QString url = _users[_currentUserId]->server(false) + "/apps/spreed";
if (!(url.contains("http://") || url.contains("https://"))) {
url = "https://" + _users[_currentUserId]->server(false) + "/apps/spreed";
auto talkApp = _users[_currentUserId]->talkApp();
Copy link
Member

Choose a reason for hiding this comment

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

I'd say use currentUser() instead of _users[_currentUserId]. We're duplicating that logic everywhere inside UserModel otherwise.

er-vin
er-vin reviewed Aug 4, 2020
View reviewed changes
src/gui/tray/UserModel.cpp Outdated
if (talkApp) {
QDesktopServices::openUrl(talkApp->url());
} else {
qCWarning(lcActivity) << "The Talk app is not enabled on " <<
Copy link
Member

Choose a reason for hiding this comment

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

Somehow didn't notice it the previous time but you don't need the space before the closing double quote. They're added for you automatically, also you can probably have this on a single line.

src/gui/tray/UserModel.cpp Outdated
QString url = _users[_currentUserId]->server(false) + "/apps/spreed";
if (!(url.contains("http://") || url.contains("https://"))) {
url = "https://" + _users[_currentUserId]->server(false) + "/apps/spreed";
auto talkApp = currentUser()->talkApp();
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: const auto?

I live by aggressively using const ;-)

Copy link
Member Author

Choose a reason for hiding this comment

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

"aggressively using const" would be a great tattoo or sticker.

er-vin
er-vin reviewed Aug 4, 2020
View reviewed changes
src/gui/tray/UserModel.cpp Outdated
@@ -871,9 +863,10 @@ void UserAppsModel::buildAppList()
}

if (UserModel::instance()->appList().count() > 0) {
auto talkApp = UserModel::instance()->currentUser()->talkApp();
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: const auto

@er-vin
Copy link
Member

er-vin commented Aug 4, 2020

/rebase

Camila added 2 commits August 4, 2020 16:28
@er-vin
Fix #2243: use server url instead of hard coded string.
6d6cc38 
Signed-off-by: Camila <hello@camila.codes>
@er-vin
Remove extra check for talk app on server in AccountState.
8f300ff 
- Remove repeated hard coded "spreed" string.

Signed-off-by: Camila <hello@camila.codes>
@github-actions github-actions bot force-pushed the fix-issue-2243 branch from 2179b45 to 8f300ff Compare