Skip to content

add option to set custom source paths#497

Merged
milianw merged 2 commits intomasterfrom
set-source-path
Jul 21, 2023
Merged

add option to set custom source paths#497
milianw merged 2 commits intomasterfrom
set-source-path

Conversation

@lievenhey
Copy link
Contributor

@lievenhey lievenhey commented Jul 7, 2023

this patch add allows the user to add a custom source path which will overwrite the source path provided in the binary
this makes the disassembler, ... work if the source code directory was moved after creating the binary

closes: #418

@GitMensch
Copy link
Contributor

GitMensch commented Jul 14, 2023 •
edited
Loading

Tested with the CI result:

  • opened recording
  • used Disassembly on a function - after a while that was shown, but not source, the source reference pointed to a directory not existing any more
  • gone to settings, added the new source path (select, then add, see it in the list) - Disassembly view was not refreshed
  • used Disassembly on a different function - the source reference still points to the same not existing directory, not the configured one where the source exists
  • used Disassembly on a different function which has its source folder unchanged, works
  • restarted HotSpot, sourcePath was not restored from last session, stopped it
  • started the AppImage ./hotspot-v1.4.1-74-ge66d066-x86_64.AppImage --sourcePaths=/my/source, got an error

    hotspot: Unknown option 'sourcePaths'.

So sadly: not successful.
When it works it seems to like this would close #482 (currently not GH-related).

@lievenhey
Copy link
Contributor Author

lievenhey commented Jul 20, 2023

thanks for testing. I fixed the --sourcePaths option and added reloading if sourcePaths changes

@GitMensch
Copy link
Contributor

GitMensch commented Jul 20, 2023

fixed the --sourcePaths option and added reloading if sourcePaths changes

Thanks, this works nearly as expected now.
It seems that while the entries that were included in the GUI are not shown in the configuration after a reload of the application, they are still active (which is not necessarily bad, I actually prefer those to be saved) - maybe they are needed to be included when the setting page is opened?

Not sure how you'd show "persistent" and "per session" entries there (I'd personally mostly need "per session" or "per configuration file", because different projects have the same binary name but the source is in different directories) .

Note that in any case this PR missed to add the option (now also in --help) to https://github.com/KDAB/hotspot/blob/master/README.md#command-line-options

@milianw milianw force-pushed the set-source-path branch 2 times, most recently from f148161 to a25e6c8 Compare July 21, 2023 12:58
lievenhey added 2 commits July 21, 2023 14:58
@lievenhey @milianw
add option to set custom source paths
8a361d9 
this patch add allows the user to add a custom source path which will
overwrite the source path provided in the binary
this makes the disassembler, ... work if the source code directory was
moved after creating the binary

closes: #418
@lievenhey @milianw
update Readme with source path
8b25ba2
@milianw
Copy link
Collaborator

milianw commented Jul 21, 2023

@GitMensch I think the per-session vs. user settings is a general issue we need to tackle somehow - can you please open a ticket to track that?

the rest of this pull request is now mergeable I believe

@milianw milianw merged commit 35871ad into master Jul 21, 2023
@milianw milianw deleted the set-source-path branch July 21, 2023 13:14
@GitMensch GitMensch mentioned this pull request Aug 16, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@milianw milianw milianw approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

FR: add option to set source paths

3 participants

@lievenhey @GitMensch @milianw