Skip to content

Add logging to MapView delegate callbacks and camera updates. #83

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

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

bolsinga
Copy link
Contributor

  • Log MLNMapViewDelegate, UIViewControllerRepresentable, MLNMapViewCameraUpdating callbacks
  • Assists newcomers to the codebase.

Issue/Motivation

  • Add logging to assist debugging and help newcomers to the codebase.

What issue is this PR targeting?
n/a

Tasklist

  • Include tests (if applicable) and examples (new or edits)
  • If there are any visual changes as a result, include before/after screenshots and/or videos
  • Add #fixes with the issue number that this PR addresses
  • Update any documentation for affected APIs

@ianthetechie ianthetechie requested review from ianthetechie and Archdoog and removed request for ianthetechie April 30, 2025 02:48
Copy link
Collaborator

@Archdoog Archdoog left a comment

Choose a reason for hiding this comment

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

One small comment. Otherwise fine with this once we sort out the test failures.

@hactar
Copy link
Collaborator

hactar commented Apr 30, 2025

Thanks! A centralized logging approach could prevent devs having to attach their own "didSet" print statements when debugging, so I'm fine with this. But:

If I'm reading the code correctly this would lead to info level logs being on per default? Personally I'm not a fan of frameworks filling my console with their own logging messages and that being the default, if all spms did that, my console would very full and I'd have to set up filtering every time before a debugging session.

I'd prefer this to be something thats toggled on or off by some init parameter, environment variable, static shared var config, etc...

@bolsinga
Copy link
Contributor Author

Thanks! A centralized logging approach could prevent devs having to attach their own "didSet" print statements when debugging, so I'm fine with this. But:

If I'm reading the code correctly this would lead to info level logs being on per default? Personally I'm not a fan of frameworks filling my console with their own logging messages and that being the default, if all spms did that, my console would very full and I'd have to set up filtering every time before a debugging session.

I'd prefer this to be something thats toggled on or off by some init parameter, environment variable, static shared var config, etc...

Hi! Thanks. Until the new unified logger, I shared your opinion. However, Xcode and Console (and release versions of the app in the wild) now can all have logging enabled, not have a performance hit, and each individual developer may use Xcode or Console to disable the logs they do not want to see when the existing UI. It also doesn't need redundant configuration in the code.

I will, of course, leave this decision up to the community. I personally think the Logger library and developer tools already allow developers to disable seeing any logs using standard UI, instead of complicating things by adding custom ways of accomplishing the same.

@ianthetechie
Copy link
Collaborator

Carrying over the question from Slack, I am able to reproduce the same test failures that show up in CI. I'm running macOS Sequoia 15.4.1 with Xcode Version 16.3 (16E140). Perhaps there is some environmental difference?

In addition to the snapshot mismatches, I receive the following fatal error in the middle of a test execution:

Thread 1: Fatal error: No return value found for member "activityIdentifier" of "MockMLNMapViewCameraUpdating" with parameter conditions: no parameters At least one return value of type "String" must be provided matching the given parameters. Use a "given" clause to provide return values.

The stack tracy is pretty deep, but it looks like the first bit of "our" code is in some Mockable-generated code for the MLNMapViewUpdating protocol.

Basically, the tests are all in checking camera updates, and that's one of the places we added logging. But the PR didn't specify what value activityIdentifier should produce, so we just need to add some Mock instructions for it in the tests. Fixing this clears up half of the failing tests.

The remaining failures are snapshot mismatches.

In this case, the easiest method is actually to just set the snapshots to record mode temporarily, re-run, and look at the diff via your favorite tool. The Xcode integration for diff display in unusable.

-         assertSnapshot(of: camera, as: .dump)
+         assertSnapshot(of: camera, as: .dump, record: true)

These all look pretty harmless. The reason all the snapshots changed is because we're using the .dump mode, and this PR changes the string representation of the types in question.

I've pushed changes that should address all these.

Copy link
Collaborator

@ianthetechie ianthetechie left a comment

Choose a reason for hiding this comment

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

I more or less agree with @bolsinga's assessment of the current generation of logging tools within Xcode.

Example 1: when you click the filter button, you get an easy way to configure the minimum log level.

image

Example 2: you can go even further and drill down into specific areas.

image

That said, it is my personal opinion that we should reduce the log level to debug rather than info. That's a matter of preference and I won't withhold and approval if there's a strong opinion to use info, but my recommendation is to downgrade these to debug.

@hactar
Copy link
Collaborator

hactar commented May 1, 2025

I honestly strongly disagree here - I quickly launched this branch in my app, which normally has a near silent app launch, and am greeted with this:

Screenshot 2025-05-01 at 07 56 54

For a MapLibre DSL contributor this may be useful information - someone trying this package for the first time, and just expecting a working package, will not have as much fun with this. Sure, I know where and how I can filter it out, but that burden should not be put on package users.

I have 32 SPMs in the current app I'm working on. Not one has the audacity to put things into my console without some form of debug flag - unless it's a major error. Apple's open source frameworks don't. Firebase doesn't. And when frameworks start doing that, people complain:

etc.

I did however find a common pattern in those libaries that do offer modern logging: passing a (default nil) Logger during init. For example (https://github.com/swift-server/async-http-client):

extension HTTPClient {
    /// Execute arbitrary HTTP requests.
    ///
    /// - Parameters:
    ///   - request: HTTP request to execute.
    ///   - deadline: Point in time by which the request must complete.
    ///   - logger: The logger to use for this request.
    ///
    /// - warning: This method may violates Structured Concurrency because it returns a `HTTPClientResponse` that needs to be
    ///            streamed by the user. This means the request, the connection and other resources are still alive when the request returns.
    ///
    /// - Returns: The response to the request. Note that the `body` of the response may not yet have been fully received.
    public func execute(
        _ request: HTTPClientRequest,
        deadline: NIODeadline,
        logger: Logger? = nil
    ) async throws -> HTTPClientResponse {

This way there is no flag, but per default the framework is silent, and if a developer wants it, they can pass in a logger, configured to their liking. Would this perhaps be a compromise @bolsinga ?

@bolsinga
Copy link
Contributor Author

bolsinga commented May 1, 2025

That said, it is my personal opinion that we should reduce the log level to debug rather than info. That's a matter of preference and I won't withhold and approval if there's a strong opinion to use info, but my recommendation is to downgrade these to debug.

I have changed them to debug level.

@bolsinga
Copy link
Contributor Author

bolsinga commented May 1, 2025

This way there is no flag, but per default the framework is silent, and if a developer wants it, they can pass in a logger, configured to their liking. Would this perhaps be a compromise @bolsinga ?

Thanks! I had been strongly in the no logs camp until the new logger. I understand all the sentiments expressed therein.

With today's tools, my personal opinion is that is is more of a burden to a newcomer to have to change the code to log what is happening than to just use the tools we already have without making any code changes.

So again, as a newcomer to this repository, I'm going to let the community decide what should be done here. I can keep a local branch with logging enabled while I come up to speed. Thanks!

@ianthetechie
Copy link
Collaborator

I just gave it a try with the debug filter, assuming that it would behave like most other logging systems and not enable logging by default, but that seems to not be the case. I'm also mildly surprised that Xcode doesn't default to limiting logs to your app's packages by default. So I can totally understand @hactar's position on this.

It doesn't really bother me a lot, and Apple is starting to have more of their logs show up already, but at the same time since this is such a new feature, I think mast iOS developers don't know it exists and will be pretty quickly annoyed. As such, I'd suggest making the logging opt-in (I don't have an opinion on how this is achieved; could be a constructor arg as suggested or something else).

@bolsinga
Copy link
Contributor Author

bolsinga commented May 9, 2025

I'll keep this on my local branch for now, and I'll work on making the logging opt in. Thanks for your feedback @ianthetechie & @hactar!

@bolsinga bolsinga marked this pull request as draft May 9, 2025 17:01
bolsinga added a commit to bolsinga/swiftui-dsl that referenced this pull request Jun 8, 2025
- Just take the `CustomStringConvertible` changes from maplibre#83
@bolsinga bolsinga mentioned this pull request Jun 8, 2025
3 tasks
ianthetechie pushed a commit that referenced this pull request Jun 10, 2025
- Just take the `CustomStringConvertible` changes from #83
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.

4 participants