Android progress indication and refactor of progress logic #10
Conversation
|
Someone is attempting to deploy this pull request to the software-mansion Team on Vercel. To accomplish this, the commit author's email address needs to be associated with a GitHub account. Learn more about how to change the commit author information. |
There was a problem hiding this comment.
This change seem to be overly complex as to what we are trying to achieve. The only thing we wanted if I understand this correctly, was for the progress listener not to trigger projectstate update in case we are not at the stage the listener belongs to. IMO this should only require a few lines of changes in project.ts to inject stage information into the listener and then to compare the current stage in that listener prior to updating project state.
| private completedTasks: number = 0; | ||
| private tasksToComplete: number = 0; | ||
|
|
||
| constructor(private progressListener: (newProgress: number, stage: number) => void) {} |
There was a problem hiding this comment.
This is another example of abstraction leak. The build now needs to understand the concept of stages and that it is a part of one of the stages in order to call this callback.
This seems unnecessary however, because the called that calls build stage knows about the stages and can inject stage number into the listener it provides for the build stage.
| this.updateProjectState({ | ||
| status: "starting", | ||
| stage: Stages.findIndex((item) => item.name === "Restarting"), | ||
| stageProgress: new Array(Stages.length).fill(0), |
There was a problem hiding this comment.
it doesn't seem correct that we send progress for all stages here, we dont need this information on the frontend
| const waitForMetro = this.metro.start( | ||
| forceCleanBuild, | ||
| (newStageProgress: number, stage: number) => { | ||
| this.stageProgressListener(newStageProgress, stage); |
There was a problem hiding this comment.
we don't need to have the arrow function pass listener here because we know which stage we are at, so we should call this.stageProgressListsner(newStageProgress, BUNDLE_STAGE)
| public async stageProgressListener(newStageProgress: number) { | ||
| if (newStageProgress < this.projectState.stageProgress) { | ||
| public async stageProgressListener(newStageProgress: number, stage: number) { | ||
| if (newStageProgress < this.projectState.stageProgress[stage]) { |
There was a problem hiding this comment.
this seems overly complicated, we don't need an array of progress here.
filip131311
force-pushed
the
@Filip131311/AndroidProgressIndication
branch
from
8b0b14d
to
0034ccd
Compare
Before:
There was no indication about the progress of android build.
Now:
Progress indicator based on number of tasks that were completed, and information about task graph.
Screen.Recording.2024-03-11.at.16.10.38.mov
Solution:
To achieve this effect I parse logs from build process to check if new tasks are being completed;
to know how many there is I added a gradle init script that logs that information.
Additionally:
This PR lets you define to which stage stageProgressListener belongs.