-
Notifications
You must be signed in to change notification settings - Fork 2
Fixed projects list on landscape iphone #118
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
Conversation
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.
Code review by ChatGPT
@@ -1801,7 +1801,7 @@ | |||
buildSettings = { | |||
BUNDLE_LOADER = "$(TEST_HOST)"; | |||
CODE_SIGN_STYLE = Automatic; | |||
CURRENT_PROJECT_VERSION = 1; | |||
CURRENT_PROJECT_VERSION = 21; | |||
DEAD_CODE_STRIPPING = YES; | |||
GENERATE_INFOPLIST_FILE = YES; | |||
MACOSX_DEPLOYMENT_TARGET = 14.0; |
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.
Thank you for submitting your pull request. I’ve reviewed the changes, focusing on the updates to the CURRENT_PROJECT_VERSION
. Here are my observations and suggestions:
1. Consistency Across Targets
- Issue: The
CURRENT_PROJECT_VERSION
is being updated from different values (1
and18
) to21
across various targets (iOS app, extensions, watch apps, macOS app, etc.). - Suggestion: Ensure that all targets should indeed share the same
CURRENT_PROJECT_VERSION
. If different targets have distinct versioning schemes, it might lead to confusion or inconsistencies in version tracking. Consider standardizing the versioning approach across all targets or clearly documenting the reasons for differing version numbers.
2. Versioning Scheme Alignment
- Issue: The jump from
18
or1
to21
is significant. It’s essential to ensure that this aligns with your project's versioning strategy (e.g., Semantic Versioning). - Suggestion:
- Semantic Versioning: If you’re following Semantic Versioning, typically, the
CURRENT_PROJECT_VERSION
(often used as the build number) increments with each build, while theMARKETING_VERSION
reflects major, minor, and patch versions. Ensure that your increments correspond to meaningful changes. - Build Automation: To minimize manual errors, consider automating the build number increment using scripts or tools like agvtool or third-party solutions like Fastlane.
- Semantic Versioning: If you’re following Semantic Versioning, typically, the
3. Impact on Dependency Management
- Issue: Changing the
CURRENT_PROJECT_VERSION
might affect dependencies, especially if other modules or services rely on specific version numbers. - Suggestion: Verify that this version update won’t disrupt any dependencies or integrations. Communicate changes with team members who manage dependent projects or services.
4. Update Related Version Fields If Necessary
- Observation: The
MARKETING_VERSION
remains at1.0
for some targets. - Suggestion: Ensure that the
MARKETING_VERSION
aligns with theCURRENT_PROJECT_VERSION
where appropriate. While they serve different purposes (public version vs. build number), maintaining a coherent relationship can aid in version tracking and user communication.
5. Documentation and Changelog Updates
- Issue: Significant version changes should be reflected in your project’s documentation and changelog.
- Suggestion:
- Changelog: Update the changelog to record the version bump from
18
or1
to21
, including the reasons for the change. - Documentation: Ensure any architecture or deployment documentation reflects the new versioning scheme.
- Changelog: Update the changelog to record the version bump from
6. Validation and Testing
- Issue: Ensuring that the version change doesn’t inadvertently affect the build or deployment process.
- Suggestion:
- CI/CD Pipelines: Verify that your continuous integration and deployment pipelines handle the new
CURRENT_PROJECT_VERSION
correctly. - Beta Testing: If applicable, test the versioned builds in beta environments to ensure stability.
- CI/CD Pipelines: Verify that your continuous integration and deployment pipelines handle the new
7. Code Signing and Entitlements
- Observation: While not directly related to the version change, maintaining consistency in
CODE_SIGN_ENTITLEMENTS
and other build settings is crucial. - Suggestion: Double-check that entitlements and code signing configurations remain correct after the version updates to prevent any signing issues.
8. Potential for Centralized Configuration
- Issue: Multiple targets have similar build settings, leading to repetitive updates.
- Suggestion:
- Xcode Configuration Files: Consider using Xcode configuration files (
.xcconfig
) to centralize shared build settings. This approach reduces redundancy and simplifies updates across multiple targets. - Swift Package Manager or Other Tools: If applicable, leverage package managers or build tools to manage shared configurations.
- Xcode Configuration Files: Consider using Xcode configuration files (
Summary
The primary change involves updating the CURRENT_PROJECT_VERSION
across multiple targets from 1
or 18
to 21
. Ensure that this update aligns with your versioning strategy, is consistently applied where necessary, and that any dependent processes or documentation are appropriately updated. Automating version increments and centralizing build settings can also enhance maintainability and reduce the potential for errors.
Let me know if you need further clarification or assistance!
if (tasksTodayActive.count + projectsCount) > 0 { badgeManager.setBadge(number: (tasksTodayActive.count + projectsCount)) | ||
} else { | ||
badgeManager.resetBadgeNumber() | ||
} | ||
WidgetCenter.shared.reloadAllTimelines() | ||
} | ||
#if os(macOS) |
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.
Code Review for SectionsListView
You've made some meaningful changes to the SectionsListView
, particularly in enhancing readability and adjusting layout constraints. Below are my observations, constructive criticisms, and suggestions to further improve the code:
1. Layout Adjustments with availableHeight
Changes:
- let availableHeight = max(geometry.size.height - 190, 0)
+ let availableHeight = max(geometry.size.height - 190, 30)
...
- let availableHeight = max(geometry.size.height - 330, 0)
+ let availableHeight = max(geometry.size.height - 330, 268)
Feedback:
-
Minimum Height Constraints: Introducing a minimum height of
30
and268
respectively prevents the view from collapsing too much, which is good for maintaining usability. However, it's unclear why these specific values were chosen. -
Magic Numbers: The numbers
30
and268
appear arbitrary. To enhance readability and maintainability, define these as named constants. For example:private let minHeightForWidth95: CGFloat = 30 private let minHeightForWidthFull: CGFloat = 268 // Usage let availableHeight = max(geometry.size.height - 190, minHeightForWidth95)
-
Responsive Design: Ensure these constants adapt well across different device sizes and orientations. If these values are based on design specifications, it might be helpful to reference them from a centralized configuration.
2. Replacing Ternary Operators with if-else
Statements
Changes:
- (newValue + tasksCount) > 0 ? badgeManager.setBadge(number: (newValue + tasksCount)) : badgeManager.resetBadgeNumber()
+ if (newValue + tasksCount) > 0 {
+ badgeManager.setBadge(number: (newValue + tasksCount))
+ } else {
+ badgeManager.resetBadgeNumber()
+ }
Feedback:
-
Readability: Switching from a ternary operator to an
if-else
statement improves readability, especially when performing actions rather than simple value assignments. This change enhances the clarity of the code. -
Consistency: Ensure that this style is consistently applied throughout the codebase. Avoid mixing ternary operators and
if-else
statements for similar logic to maintain uniformity. -
Simplification: If the logic becomes more complex in the future, this structure will scale better than a nested ternary.
3. DRY Principle for Badge Management
Observation:
The badge update logic is duplicated in both onChange
and onAppear
closures.
Feedback:
- Extract Common Logic: To adhere to the DRY (Don't Repeat Yourself) principle, consider extracting the badge update logic into a separate method. This reduces redundancy and makes maintenance easier.
Suggestion:
private func updateBadge(with count: Int) {
if count > 0 {
badgeManager.setBadge(number: count)
} else {
badgeManager.resetBadgeNumber()
}
WidgetCenter.shared.reloadAllTimelines()
}
Usage:
.onChange(of: projects.filter { TasksQuery.filterProjectToReview($0) }.count) { _, newValue in
if showReviewProjectsBadge {
let tasksCount = tasksTodayActive.count
updateBadge(with: newValue + tasksCount)
}
}
...
.onAppear {
var projectsCount = 0
if showReviewProjectsBadge {
projectsCount = projects.filter { TasksQuery.filterProjectToReview($0) }.count
}
updateBadge(with: tasksTodayActive.count + projectsCount)
}
4. Optimizing Widget Timeline Reloads
Observation:
WidgetCenter.shared.reloadAllTimelines()
is called after setting or resetting the badge.
Feedback:
-
Performance Consideration:
reloadAllTimelines()
reloads all widget timelines, which might be unnecessary and could impact performance. -
Scoped Reloading: If possible, reload only the specific widget that needs updating using
reloadTimelines(ofKind:)
. This approach is more efficient.
Suggestion:
WidgetCenter.shared.reloadTimelines(ofKind: "YourWidgetKind")
Replace "YourWidgetKind"
with the actual kind identifier of your widget.
5. Handling State and Data Flow
Observation:
The badge count depends on both projects
and tasksTodayActive
.
Feedback:
-
Combine Publishers: Consider using Combine to manage data flow and state updates more declaratively. This can help in managing dependencies between different pieces of state and reduce boilerplate code.
-
Performance: Continuously filtering
projects
insideonChange
andonAppear
can be optimized. Ifprojects
is large, repeated filtering might be costly.
Suggestion:
- Memoization: Cache the filtered projects if the filtering criteria don’t change frequently.
- State Variables: Use
@State
or@Published
properties to hold filtered results, updating them reactively whenprojects
or related criteria change.
6. Code Formatting and Styling
Feedback:
-
Consistent Indentation: Ensure consistent indentation for better readability. Misaligned braces or inconsistent spacing can make the code harder to read and maintain.
-
Parentheses Usage: While parentheses improve clarity, excessive use can clutter the code. Swift's operator precedence handles most cases, so use parentheses judiciously.
Example:
if tasksTodayActive.count + projectsCount > 0 {
badgeManager.setBadge(number: tasksTodayActive.count + projectsCount)
} else {
badgeManager.resetBadgeNumber()
}
7. Documentation and Comments
Feedback:
-
Explain Magic Numbers: As previously mentioned, document the reasons behind specific constants like
30
and268
. This aids future developers in understanding the intent without digging through UI designs. -
Method Documentation: If you extract methods (e.g.,
updateBadge
), add comments explaining their purpose, parameters, and usage scenarios.
8. Testing Considerations
Feedback:
-
Unit Tests: Ensure that there are unit tests covering the badge update logic, especially the conditions under which badges are set or reset.
-
UI Tests: Verify that the layout adjustments behave as expected across different device sizes and orientations.
Summary
Your changes improve code clarity by replacing ternary operators with if-else
statements and introduce sensible constraints to the view's layout. To further enhance the code:
- Avoid Magic Numbers: Use named constants and document their purpose.
- Extract Repeated Logic: Create helper methods for repeated operations like badge updates.
- Optimize Performance: Limit widget timeline reloads to specific widgets and optimize data filtering.
- Enhance Readability: Maintain consistent code formatting and style.
- Implement Testing: Ensure robust testing to cover new changes and logic.
Incorporating these suggestions will lead to a more maintainable, efficient, and readable codebase.
No description provided.