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

Rework messages to centralized approach #1109

Merged
merged 5 commits into from
Sep 14, 2024

Conversation

wutschel
Copy link
Collaborator

Description

This PR reworks the approach on how messages are shown. Before, each UIViewController had an own instance of MessagesView and the messages were presented by it. Now, there is only a single instance of MessagesView in either the iPad's or iPhone's main controller. All other UIVewControllers send notifications which are consumed by this central instance and shown from there. This allows to reduce code to layout and create several instances and it easily aligns the layout of the messages.
In addition, this PR brings back the messages after executing action from the "Power Menu" (e.g. Audio Library Scan). Those were lost as part of refactoring and unifying the power menu.

Summary for release notes

Maintenance: Use centralized approach to show messages
Bugfix: Show power menu related messages again

@wutschel wutschel force-pushed the rework_messages_2 branch 2 times, most recently from 5c4a92b to 7d3eeb1 Compare September 14, 2024 10:01
@wutschel wutschel marked this pull request as ready for review September 14, 2024 10:03
Comment on lines 604 to 627
[[NSNotificationCenter defaultCenter] addObserver: self
selector: @selector(showNotificationMessage:)
name: @"AudioLibrary.OnScanFinished"
object: nil];

[[NSNotificationCenter defaultCenter] addObserver: self
selector: @selector(showNotificationMessage:)
name: @"AudioLibrary.OnCleanFinished"
object: nil];

[[NSNotificationCenter defaultCenter] addObserver: self
selector: @selector(showNotificationMessage:)
name: @"VideoLibrary.OnScanFinished"
object: nil];

[[NSNotificationCenter defaultCenter] addObserver: self
selector: @selector(showNotificationMessage:)
name: @"VideoLibrary.OnCleanFinished"
object: nil];

[[NSNotificationCenter defaultCenter] addObserver: self
selector: @selector(showNotificationMessage:)
name: @"UIShowMessage"
object: nil];
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • copy-pasted
  • rather loop through an array of notification names

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If you don't object, I would like to keep this. This way the notifications are all listed in one block and easy to read and oversee. If I now introduce a dedicated method or loop, it is not that straight forward anymore.

Copy link
Collaborator

Choose a reason for hiding this comment

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

not sure how using a loop makes it less straightforward :)

NSArray* notificationsWithMessage = @[
  @"AudioLibrary.OnScanFinished",
  ...
];
for (NSString* notificationWithMessage in notificationsWithMessage) {
  [NSNotificationCenter.defaultCenter addObserver: self
                                             selector:@selector(showNotificationMessage:)
                                                 name:notificationWithMessage 
                                               object:nil];
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I know what you meant, I just find this less straight forward to oversee in a block of other addObserver calls which all follow the same pattern.

Maybe we just look different at the code. :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Alternatively you can create base class for those 2 view controllers and place this code in there. Also move other common things there (I believe there're some)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, interesting thought. There really might be some common code ... Ok to do this separately?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, interesting thought. There really might be some common code ... Ok to do this separately?

Sure, that's fine

As we can now trigger library scan/clean from several view the messages are now shown by the central view controllers for iPad and iPhone. The implementation inside RightMenuVC is obsolete.
Instead of using a dedicated instance per UIViewController only send a notification which is picked by the central controllers for iPad/iPhone. This central controller will handle displaying all messages.
Move existing helper method topMostController to Utilities and re-use also for placing the messagesView.
@wutschel
Copy link
Collaborator Author

Nicely squashed and rebased.

@kambala-decapitator kambala-decapitator merged commit deedff0 into xbmc:master Sep 14, 2024
@wutschel wutschel deleted the rework_messages_2 branch September 14, 2024 20:19
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.

2 participants