-
Notifications
You must be signed in to change notification settings - Fork 33
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 a flow of loading devices and xcode detection #563
Changes from all commits
beccd30
e478f6f
d18d9f2
cccd83f
33bdf03
e86461f
d284655
c50b2f6
2326812
7294163
ffb3967
4599713
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,6 +27,7 @@ import { EventEmitter } from "stream"; | |
import { Logger } from "../Logger"; | ||
import { extensionContext } from "../utilities/extensionContext"; | ||
import { Platform } from "../utilities/platform"; | ||
import { checkXcodeExists } from "../dependency/DependencyManager"; | ||
|
||
const DEVICE_LIST_CACHE_KEY = "device_list_cache"; | ||
|
||
|
@@ -104,13 +105,20 @@ export class DeviceManager implements DeviceManagerInterface { | |
Logger.error("Error fetching emulators", e); | ||
return []; | ||
}); | ||
const simulators = | ||
Platform.OS === "macos" | ||
? listSimulators().catch((e) => { | ||
Logger.error("Error fetching simulators", e); | ||
return []; | ||
}) | ||
: Promise.resolve([]); | ||
|
||
let shouldLoadSimulators = Platform.OS === "macos"; | ||
|
||
if (!(await checkXcodeExists())) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do we even want to run those checks if we're not on a Mac? Perhaps doing it this way would be cleaner: const shouldLoadSimulators = Platform.OS === "macos" && (await checkXcodeExists()); Then you can move the log to be printed in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did this in this way to address your previous comment about adding logging in !checkXcodeExist() scennario |
||
shouldLoadSimulators = false; | ||
Logger.debug("Couldn't list iOS simulators as XCode installation wasn't found"); | ||
} | ||
|
||
const simulators = shouldLoadSimulators | ||
? listSimulators().catch((e) => { | ||
Logger.error("Error fetching simulators", e); | ||
return []; | ||
}) | ||
: Promise.resolve([]); | ||
const [androidDevices, iosDevices] = await Promise.all([emulators, simulators]); | ||
const devices = [...androidDevices, ...iosDevices]; | ||
this.eventEmitter.emit("devicesChanged", devices); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,6 +26,7 @@ const ProjectContext = createContext<ProjectContextProps>({ | |
longitude: 19.965474, | ||
isDisabled: false, | ||
}, | ||
locale: "en_US", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why is this change here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It was missed before and ts was not happy, it does not effect the webview much at all as it never uses default context. |
||
}, | ||
project, | ||
}); | ||
|
@@ -46,6 +47,7 @@ export default function ProjectProvider({ children }: PropsWithChildren) { | |
longitude: 19.965474, | ||
isDisabled: false, | ||
}, | ||
locale: "en_US", | ||
}); | ||
|
||
useEffect(() => { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,6 +20,27 @@ import { useDiagnosticAlert } from "../hooks/useDiagnosticAlert"; | |
import { ZoomLevelType } from "../../common/Project"; | ||
import { useUtils } from "../providers/UtilsProvider"; | ||
|
||
type LoadingComponentProps = { | ||
finishedInitialLoad: boolean; | ||
devicesNotFound: boolean; | ||
}; | ||
|
||
function LoadingComponent({ finishedInitialLoad, devicesNotFound }: LoadingComponentProps) { | ||
if (!finishedInitialLoad) { | ||
return ( | ||
<div className="missing-device-filler"> | ||
<VSCodeProgressRing /> | ||
</div> | ||
); | ||
} | ||
|
||
return ( | ||
<div className="missing-device-filler"> | ||
{devicesNotFound ? <DevicesNotFoundView /> : <VSCodeProgressRing />} | ||
</div> | ||
); | ||
} | ||
|
||
function PreviewView() { | ||
const { projectState, project } = useProject(); | ||
const { reportIssue } = useUtils(); | ||
|
@@ -93,14 +114,6 @@ function PreviewView() { | |
} | ||
}; | ||
|
||
if (!finishedInitialLoad) { | ||
return ( | ||
<div className="panel-view"> | ||
<VSCodeProgressRing /> | ||
</div> | ||
); | ||
} | ||
|
||
function onMouseDown(e: MouseEvent<HTMLDivElement>) { | ||
e.preventDefault(); | ||
setIsPressing(true); | ||
|
@@ -143,7 +156,7 @@ function PreviewView() { | |
</IconButton> | ||
</SettingsDropdown> | ||
</div> | ||
{selectedDevice ? ( | ||
{selectedDevice && finishedInitialLoad ? ( | ||
<Preview | ||
key={selectedDevice.id} | ||
isInspecting={isInspecting} | ||
|
@@ -154,9 +167,10 @@ function PreviewView() { | |
onZoomChanged={onZoomChanged} | ||
/> | ||
) : ( | ||
<div className="missing-device-filler"> | ||
{devicesNotFound ? <DevicesNotFoundView /> : <VSCodeProgressRing />} | ||
</div> | ||
<LoadingComponent | ||
finishedInitialLoad={finishedInitialLoad} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why we're checking for finishedInitialLoad twice in the ternary and in the component? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. there is more then one reason for rendering the loading component, but not all of them behave the same. |
||
devicesNotFound={devicesNotFound} | ||
/> | ||
)} | ||
|
||
<div className="button-group-bottom"> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: "CLI: " prefix is unnecessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is useful to have prefixes that allow to serach what part of our code generated the log. We have. a growing code base and most of that log is generated dynamically so searching for it would be hard without it.