Conversation
AWoloszyn
requested review from
ben-clayton,
pau-baiget and
pmuetschard
and removed request for
ben-clayton
|
@ben-clayton I have responded to all comments. Thanks! |
ben-clayton
left a comment
ben-clayton
left a comment
There was a problem hiding this comment.
Looks good, mostly nits.
AWoloszyn
force-pushed
the
stadia
branch
2 times, most recently
from
6bf30fc to
55ee76a
Compare
hevrard
left a comment
hevrard
left a comment
There was a problem hiding this comment.
Mostly typos and small comments, fix as you see fit.
| return ret, nil | ||
| } | ||
|
|
||
| // ParseListOutput parses the output from ggp xxx list command. |
There was a problem hiding this comment.
ggp xxx list -s gives the output as JSON, would it be better to use this and a proper JSON parser?
If we stick with plain text, maybe add a small example of the kind of text that is parsed? e.g.
blablabla
header1 header2 header3
======= ======= =======
value1 value2 value3
There was a problem hiding this comment.
I added the example, there is a good reason (for now) that we support this option. We can remove in the future. I can elaborate offline if you would like.
| // corresponding return value. One special case only used internally is: | ||
| // "package=/", which returns proj="/", pkg="", app="". | ||
| func parsePackageURI(ctx context.Context, uri string) (proj, pkg, app string, err error) { | ||
| re := regexp.MustCompile(`package=(\/$|[a-zA-Z0-9\_\-\s]+\/)?([a-z0-9\_\-\s]+)?(\:[a-zA-Z0-9\s\_\-]+$)?`) |
There was a problem hiding this comment.
Nit: not sure regexp is the most maintainable approach here, but I guess it does the job.
Another approach would be to tokenize the URI using ['/', '=', ':'] as delimiters and analyse the resulting split.
There was a problem hiding this comment.
The allowable tokens are fairly well-defined, so I think the regex is okay at the moment. If we run into issues, we can split it out more carefully.
|
Updated from comments. |
|
@pmuetschard @hevrard Would either of you be able to review/approve this? I would like to avoid waiting too much longer, and having to rebase. |
ben-clayton
left a comment
ben-clayton
left a comment
There was a problem hiding this comment.
There are still a bunch of unaddressed comments from @hevrard
core/os/device/ggp/device.go
Outdated
| var lastErrorPrinted time.Time | ||
|
|
||
| for { | ||
| if err := func() error { |
There was a problem hiding this comment.
This whole closure looks pointless.
Isn't it the same as:
configs, err := getConfigs(ctx)
if err != nil {
return err
}
if err := scanDevices(ctx, configs); err != nil {
if time.Since(lastErrorPrinted).Seconds() > printScanErrorsEveryNSeconds {
log.E(ctx, "Couldn't scan devices: %v", err)
lastErrorPrinted = time.Now()
}
} else {
lastErrorPrinted = time.Time{}
}| xcbInfo.window = info->window; | ||
| } | ||
| break; | ||
| case gapir::SurfaceType::Ggp: |
There was a problem hiding this comment.
Indentation is off, which is quite confusing.
There was a problem hiding this comment.
Still off compared to L391.
|
Thanks, I will update. They got themselves buried I guess. |
ben-clayton
left a comment
ben-clayton
left a comment
There was a problem hiding this comment.
Some nits to do with indenting still, but LGTM.
Clicking approve - let you decide whether I'm still +2 worthy. 😉
| target = gapir::SurfaceType::Xcb; | ||
| break; | ||
| case VK_STRUCTURE_TYPE_WIN32_SURFACE_CREATE_INFO_KHR: | ||
| case VK_STRUCTURE_TYPE_STREAM_DESCRIPTOR_SURFACE_CREATE_INFO_GGP: |
| switch (target) { | ||
| #if TARGET_OS == GAPID_OS_ANDROID | ||
| case gapir::SurfaceType::Android: | ||
| createInfo = &androidInfo; |
| xcbInfo.window = info->window; | ||
| } | ||
| break; | ||
| case gapir::SurfaceType::Ggp: |
There was a problem hiding this comment.
Still off compared to L391.
|
Fixed formatting, will merge once builds go green. |
hevrard
left a comment
hevrard
left a comment
There was a problem hiding this comment.
I tried to comment on the indentation but you had pushed in between!
lgtm
4 participants
No description provided.