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

AppMap "pins" can be disabled by the user #769

Closed
brikelly opened this issue Aug 4, 2023 · 3 comments
Closed

AppMap "pins" can be disabled by the user #769

brikelly opened this issue Aug 4, 2023 · 3 comments
Labels
enhancement New feature or request

Comments

@brikelly
Copy link
Contributor

brikelly commented Aug 4, 2023

Issue

Given that source code files can be changed after maps are generated, this can lead to the pink "pins" being placed incorrectly within the file viewer.

Therefore we will add an extension setting that allows a user to disable the display of pins if they so choose. It will remain enabled by default when the extension is first installed.

Example pin in source code

Image

Where the AppMap pin enabled setting should be located

Image

Details

  • Add the setting to the configuration field in package.json.
  • Here is where the code for the pin is located.
  • Please add a test (either unit or integration, but please do not add a system test).
@brikelly brikelly added the enhancement New feature or request label Aug 4, 2023
@kgilpin
Copy link
Contributor

kgilpin commented Sep 8, 2023

@brikelly I'm not sure that this is the best approach. How are users going to know about this option? And then they have to disable the pins for the entire project - not just the files that are out of sync.

I suggest a different approach that I think is better. We know the timestamp of the AppMaps that include the source file. If the modified date of the source file is later than the timestamp of any of the AppMaps, don't show the Pin.

@brikelly
Copy link
Contributor Author

brikelly commented Sep 8, 2023

@kgilpin yeah that's a better approach. Although we should decide if we're going to keep the pins at all first.

@brikelly
Copy link
Contributor Author

Resolved in release 0.101.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants