-
Notifications
You must be signed in to change notification settings - Fork 353
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
Do not show change log when it is empty #5297
Do not show change log when it is empty #5297
Conversation
IOS-346 Stop showing the changelog to users when it is empty
When there are no meaningful changes we want to advertise in the app. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed all commit messages.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @rablador)
ios/MullvadVPN/Coordinators/ApplicationCoordinator.swift
line 998 at r1 (raw file):
fileprivate extension AppPreferencesDataSource { var isSeenLatestChanges: Bool {
Can we rename this hasSeenLastChanges
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @rablador)
ios/MullvadVPN/Coordinators/ApplicationCoordinator.swift
line 998 at r1 (raw file):
Previously, buggmagnet wrote…
Can we rename this
hasSeenLastChanges
?
Or hasSeenLastChangelog
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @rablador)
ios/MullvadVPN/Coordinators/ApplicationCoordinator.swift
line 998 at r1 (raw file):
Previously, buggmagnet wrote…
Or
hasSeenLastChangelog
With the other suggestion, we can simplify this to the following (which is IMHO way easier to understand)
var hasSeenLastChanges: Bool {
ChangeLogInteractor().hasNewChanges() ||
lastSeenChangeLogVersion != Bundle.main.shortVersion
}
ios/MullvadVPN/View controllers/ChangeLog/ChangeLogInteractor.swift
line 14 at r1 (raw file):
final class ChangeLogInteractor { private let logger = Logger(label: "ChangeLogInteractor") private(set) var items: [String] = []
We shouldn't be exposing the guts of the ChangeLogInteractor
for no reason.
Instead, it should say itself whether there are changes or not.
here's a suggestion
public func hasNewChanges() -> Bool {
items.isEmpty == false
}
260cbc4
to
efaee83
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @rablador)
ios/MullvadVPN/Coordinators/ApplicationCoordinator.swift
line 998 at r1 (raw file):
Previously, buggmagnet wrote…
With the other suggestion, we can simplify this to the following (which is IMHO way easier to understand)
var hasSeenLastChanges: Bool { ChangeLogInteractor().hasNewChanges() || lastSeenChangeLogVersion != Bundle.main.shortVersion }
Correction
var hasSeenLastChanges: Bool {
ChangeLogInteractor().hasNewChanges() == false &&
lastSeenChangeLogVersion == Bundle.main.shortVersion
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @buggmagnet)
ios/MullvadVPN/Coordinators/ApplicationCoordinator.swift
line 998 at r1 (raw file):
Previously, buggmagnet wrote…
With the other suggestion, we can simplify this to the following (which is IMHO way easier to understand)
var hasSeenLastChanges: Bool { ChangeLogInteractor().hasNewChanges() || lastSeenChangeLogVersion != Bundle.main.shortVersion }
Indeed, much better!
ios/MullvadVPN/View controllers/ChangeLog/ChangeLogInteractor.swift
line 14 at r1 (raw file):
Previously, buggmagnet wrote…
We shouldn't be exposing the guts of the
ChangeLogInteractor
for no reason.
Instead, it should say itself whether there are changes or not.here's a suggestion
public func hasNewChanges() -> Bool { items.isEmpty == false }
I can meet you just past halfway.
efaee83
to
e36288f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 2 files at r3, all commit messages.
Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @rablador)
ios/MullvadVPN/Coordinators/ApplicationCoordinator.swift
line 998 at r1 (raw file):
Previously, rablador (Jon Petersson) wrote…
Indeed, much better!
The parenthesis were not necessary. But not the end of the world.
e36288f
to
d903827
Compare
When there are no meaningful changes we do not want to advertise anything in the app.
This change is