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

Improve Attributions #1390

Closed
wants to merge 4 commits into from
Closed

Conversation

ianthetechie
Copy link
Contributor

@ianthetechie ianthetechie commented Oct 24, 2022

See #1379.

This PR adds a new attribution control, RichAttributionWidget as an alternative to the existing AttributionWidget (which is still valid if you want a very minimal style) and has a more expressive API built around SourceAttribution and its concrete subclasses.

  • Toggle full display of non-logo attributions via the info button
  • Concrete implementations for text and logo attributions
  • Ability to do "hyperlink" style URI launching for all attribution types, while still enabling a completely custom onTap handler for the few that want that.

Possible discussion points:

  • Logo attributions are always visible; I have yet to see a case where this isn't required in my map work but the API could be extended if desired
  • I opted NOT to change the opacity of the logos as in a prototype passed around on Discord because my understanding is that this could be a big performance impact.
  • In my understanding, the current attribution widget implementation is implemented in a very strange way. The default static helper doesn't even construct an attribution widget. I fixed this. I additionally think it should actually just be implemented as a named constructor rather than a static method, but open to opinions.

TODOs:

  • Decide if we want any more logo attribution constructors (I just implemented the obviously necessary ones that I cared about)
  • Tests (probably?)
  • This project always gets a logo attribution if the control is added. There should probably be a text option instead (discussion welcome)? I also don't know where the official logo is hosted so I just grabbed it from an HTML inspection of the docs. This probably needs to change.
  • Probably a version bump (not sure project policy on this)

Screenshot gallery:

image

image

@github-actions
Copy link

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@github-actions github-actions bot added the Stale label Nov 25, 2022
@JaffaKetchup JaffaKetchup self-assigned this Dec 20, 2022
@github-actions
Copy link

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@JaffaKetchup
Copy link
Member

Superseded/continued/replaced by #1487.

@fleaflet fleaflet locked and limited conversation to collaborators Apr 12, 2023
@JaffaKetchup JaffaKetchup deleted the attribution-poc branch April 14, 2023 20:25
@ibrierley
Copy link
Collaborator

Note, I sometimes get

E/flutter (29292): [ERROR:flutter/runtime/dart_vm_initializer.cc(41)] Unhandled Exception: Null check operator used on a null value
E/flutter (29292): #0      State.setState (package:flutter/src/widgets/framework.dart:1153)
E/flutter (29292): #1      RichAttributionWidgetState.initState.<anonymous closure> (package:flutter_map/src/layer/attribution_layer/rich.dart:168)
E/flutter (29292): #2      new Future.delayed.<anonymous closure> (dart:async/future.dart:424)
E/flutter (29292): #3      Timer._createTimer.<anonymous closure> (dart:async-patch/timer_patch.dart:18)
E/flutter (29292): #4      _Timer._runTimers (dart:isolate-patch/timer_impl.dart:398)
E/flutter (29292): #5      _Timer._handleMessage (dart:isolate-patch/timer_impl.dart:429)
E/flutter (29292): #6      _RawReceivePort._handleMessage (dart:isolate-patch/isolate_patch.dart:192)

I'm currently testing the new v4 code, but I suspect this is more related to this (sorry a little out of touch atm)?

Struggling to find a good method of reproducing it though...

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants