martin-g commented on code in PR #1828:
URL:
https://github.com/apache/datafusion-ballista/pull/1828#discussion_r3370013969
##########
ballista-cli/src/tui/app.rs:
##########
@@ -221,13 +221,47 @@ impl App {
_ => {}
}
} else if popup.is_plan_view() {
- match key.code {
- KeyCode::Esc => popup.set_no_details_view(),
- KeyCode::Up => popup.scroll_up(),
- KeyCode::Down => popup.scroll_down(),
- KeyCode::Left => popup.scroll_left(),
- KeyCode::Right => popup.scroll_right(),
- _ => {}
+ use crate::tui::domain::jobs::stages::StagePlanTab;
+
+ // Collect job_id + tab to fetch while popup is still borrowed,
+ // then drop the borrow before the async call.
+ let fetch = match key.code {
+ KeyCode::Char('d') => popup
Review Comment:
You will have to do the same for the Web TUI.
At the moment the key event handing is duplicated due to differences between
Crossterm/Ratzilla event handling (async/sync) :-/
##########
ballista-cli/src/tui/app.rs:
##########
@@ -797,6 +831,11 @@ impl App {
UiData::JobStagesData(job_id, stages) => {
self.job_stages_popup = Some(JobStagesPopup::new(job_id,
stages));
}
+ UiData::JobStagesPlanData(_job_id, tab, stages) => {
Review Comment:
I was going to suggest to not pass `job_id` if it won't be used but I think
it might be better to check that it is the id of the currently edited job. To
prevent a late Ajax response modifying something unrelated.
##########
ballista-cli/src/tui/domain/jobs/stages.rs:
##########
@@ -102,6 +122,36 @@ impl JobStagesPopup {
}
}
+ pub fn cache_plan_response(&mut self, fmt: StagePlanTab, resp:
JobStagesResponse) {
+ match fmt {
+ StagePlanTab::Default => self.plan_cache.default =
Some(resp.clone()),
+ StagePlanTab::Tree => self.plan_cache.tree = Some(resp.clone()),
+ StagePlanTab::Metrics => self.plan_cache.metrics =
Some(resp.clone()),
+ }
+ // If we're currently on that tab, update the live stages too so the
+ // selection / scroll state is preserved.
+ let active_fmt = self.active_plan_format();
+ if active_fmt == Some(fmt) {
+ self.stages = resp;
+ }
+ }
Review Comment:
The cloning can be avoided if the tab is not the current active one:
```suggestion
pub fn cache_plan_response(&mut self, fmt: StagePlanTab, resp:
JobStagesResponse) {
let active_fmt = self.active_plan_format();
if active_fmt.as_ref() == Some(&fmt) {
self.stages = resp.clone();
}
match fmt {
StagePlanTab::Default => self.plan_cache.default = Some(resp),
StagePlanTab::Tree => self.plan_cache.tree = Some(resp),
StagePlanTab::Metrics => self.plan_cache.metrics = Some(resp),
}
}
```
##########
ballista-cli/src/tui/domain/jobs/stages.rs:
##########
@@ -92,6 +108,10 @@ impl JobStagesPopup {
Self {
job_id,
scrollbar_state: ScrollbarState::new(stages.stages.len()),
+ plan_cache: PlanCache {
+ default: Some(stages.clone()),
Review Comment:
This is not always true.
There is an TUI config `job.stage.plan.tree` that may say that the `tree`
rendering is the default.
We can drop the config now since it will be very easy to change the plan
rendering type.
##########
ballista-cli/src/tui/ui/main/jobs/mod.rs:
##########
@@ -102,6 +103,40 @@ pub async fn load_job_stages_popup(app: &App, job_id:
&str) -> TuiResult<()> {
.await
}
+/// Loading stage's plan to render the popup window
+#[cfg(not(feature = "web"))]
+pub async fn load_stage_plan(
+ app: &App,
+ job_id: &str,
+ tab: crate::tui::domain::jobs::stages::StagePlanTab,
+) -> TuiResult<()> {
+ use crate::tui::domain::jobs::stages::StagePlanTab;
+
+ let fmt = match tab {
+ StagePlanTab::Default => None,
Review Comment:
```suggestion
StagePlanTab::Default => Some(""),
```
or `Some("default")`
to bypass the TUI config setting `job.stage.plan.tree`
##########
ballista-cli/src/tui/domain/jobs/stages.rs:
##########
@@ -102,6 +122,36 @@ impl JobStagesPopup {
}
}
+ pub fn cache_plan_response(&mut self, fmt: StagePlanTab, resp:
JobStagesResponse) {
+ match fmt {
+ StagePlanTab::Default => self.plan_cache.default =
Some(resp.clone()),
+ StagePlanTab::Tree => self.plan_cache.tree = Some(resp.clone()),
+ StagePlanTab::Metrics => self.plan_cache.metrics =
Some(resp.clone()),
+ }
+ // If we're currently on that tab, update the live stages too so the
+ // selection / scroll state is preserved.
+ let active_fmt = self.active_plan_format();
+ if active_fmt == Some(fmt) {
+ self.stages = resp;
+ }
+ }
+
+ #[allow(dead_code)]
+ pub fn cached_response(&self, tab: &StagePlanTab) ->
Option<JobStagesResponse> {
+ match tab {
+ StagePlanTab::Default => self.plan_cache.default.clone(),
+ StagePlanTab::Tree => self.plan_cache.tree.clone(),
+ StagePlanTab::Metrics => self.plan_cache.metrics.clone(),
+ }
+ }
Review Comment:
Better return a reference
```suggestion
pub fn cached_response(&self, tab: &StagePlanTab) ->
Option<&JobStagesResponse> {
match tab {
StagePlanTab::Default => self.plan_cache.default.as_ref(),
StagePlanTab::Tree => self.plan_cache.tree.as_ref(),
StagePlanTab::Metrics => self.plan_cache.metrics.as_ref(),
}
}
```
##########
ballista-cli/src/tui/ui/footer.rs:
##########
@@ -80,6 +80,12 @@ pub(super) fn render_footer(f: &mut Frame, area: Rect, app:
&App) {
} else if app.is_job_stage_plan_popup_open() {
current_view_key_bindings
.push(Span::from("[↑↓] Scroll up/down, "));
Review Comment:
I see we missed to add the horizontal scrolling binding here.
Please add it too!
```suggestion
.push(Span::from("[↑↓←→] Scroll, "));
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
