flutter / devtools

Performance tools for Flutter
https://flutter.dev/docs/development/tools/devtools/
BSD 3-Clause "New" or "Revised" License
1.53k stars 313 forks source link

Deep Links tool shows incorrect path and fails to validate on Windows #7781

Open DanTup opened 1 month ago

DanTup commented 1 month ago

When I open the Deep Links tool in VS Code, the populated path is incorrect for Windows (it has a leading slash and all slashes are the wrong direction):

image

Clicking the button results in this message:

image

Tested with Flutter 3.22.0 and master (although that claims to be 3.22.0-36.0.pre.56 which seems odd, because that would put it behind stable).

kenzieschmoll commented 1 month ago

The roots are pulled from DTD here: https://github.com/flutter/devtools/blob/master/packages/devtools_app/lib/src/screens/deep_link_validation/project_root_selection/select_project_view.dart/#L43.

So perhaps there is a bug in package:dtd? CC @CoderDake The project roots are determined by looking at the workspace roots and grabbing every directory with a pubspec.yaml file: https://github.com/dart-lang/sdk/blob/main/pkg/dtd_impl/lib/src/service/file_system_service.dart#L108

kenzieschmoll commented 1 month ago

We should have testing for package:dtd on a Windows bot if we do not already.

DanTup commented 1 month ago

My guess is that the (first) issue is this uri.path here:

https://github.com/flutter/devtools/blob/6738316652b01c35ccd1630711e1b101b6694ac4/packages/devtools_app/lib/src/screens/deep_link_validation/project_root_selection/root_selector.dart#L120

Calling .path on a Windows file URI like file:///c:/foo will result in /c:/foo and not C:\foo. Generally we should use .toFilePath(), however that also won't work in the browser because the browser always pretends to be non-Windows (see notes at https://github.com/flutter/devtools/blob/master/STYLE.md#uris-and-file-paths).

Probably we would need to do something similar to here, although it's harder to guess whether the URI is Windows or not (compared to doing the other way, where we can just check the slash directions). Possibly we could check if the .path starts with /[a-zA-Z]:/ and then use the result as the windows argument?

https://github.com/flutter/devtools/blob/6738316652b01c35ccd1630711e1b101b6694ac4/packages/devtools_app/lib/src/standalone_ui/vs_code/debug_sessions.dart#L160-L167

CoderDake commented 1 month ago

Would it be worth us using pkg:path's Builder in order to create a windows style path? It looks like that behaviour allows you to work with the path of a specific platform. For example windows would be: path.Builder(style: Style.windows); https://distributed-dart.github.io/docs/path.html

DanTup commented 1 month ago

@CoderDake would that be any different to just passing the windows flag to toFilePath()?

CoderDake commented 1 month ago

Sorry I assumed that the goal was to get a clean windows path.

Yes it looks like they behave differently. If we use pkg:path with a windows style then we get no leading slash along with windows slashes afterwards.

I quickly made a dartpad to show the difference

Screenshot 2024-05-21 at 2 59 16 PM
DanTup commented 1 month ago

@CoderDake I'm not sure I understand the example. If you change the last line to print uri.toFilePath(windows: true) it should return what we want (C:\xxx\yyy).

Normally, the windows argument would not be required, however in the web context, paths are always treated as non-Windows so it must be passed (if the URI represents a Windows file path). However, we'd need to know what to pass as the value of windows, and that means we need to examine the URI and guess/assume that it's a Windows path based on the content.

When going from file path -> URI, it's easy, because the file path will have \ on Windows. However when going from URI -> path, the slashes are always the same, so we can probably only guess based on the presence of what looks like a drive letter? 🤔

CoderDake commented 1 month ago

ah ok my apologies for the misunderstanding!