A chart of a whole table whose new table at the same corner has other series, or a chart whose range is empty now, is removed rather than left drawing the wrong cells or nothing. The note lists the removed charts first, then the re-pointed and kept ones; undoing the replace brings the removed charts back.
- commit
- 784e49c09a6caa2e09c7c38060cf57836113f912
- parent
- ec1f34fce029b61131c0e2a396ae9ce68fef8f3d
- author
- Dami <dami.etoile@gmail.com>
| M | docs/ | 2 | +- |
| M | e2e/ | 12 | +++++++- |
| M | internal/ | 50 | +++++++++++++++++++----------- |
| M | internal/ | 18 | +++++++---- |
| M | internal/ | 28 | ++++++++++------- |
| M | internal/ | 17 | +++++----- |
@@ -13,7 +13,7 @@ File > Import asks where the data goes, as Sheets' Import location does
1313 | Location | Does |1414 |---|---|1515 | Insert new sheet(s) | Adds the file's sheets after the sheet shown: every sheet of an `.xlsx`, named after the file for other formats. A name already taken gets a number (`Sales 2`) and the file's formulas follow it; named ranges come along unless their name is taken. One undo step, and the spreadsheet stays the file you're editing |16-| Replace current sheet | Puts the data in place of the sheet shown, keeping its name and position, so formulas and named ranges that read it read the new data. Its charts stay: a chart that drew a whole table is re-pointed to the table the file has at the same corner when it has as many columns (rows for a chart by row), so last month's chart draws this month's rows; any other chart keeps its range. The context line says which chart was re-pointed, which kept its range, and which range is empty now. One undo step. Not offered for `.xlsx`, which holds several sheets |16+| Replace current sheet | Puts the data in place of the sheet shown, keeping its name and position, so formulas and named ranges that read it read the new data. Its charts that fit the new data stay: a chart that drew a whole table is re-pointed to the table the file has at the same corner when it has as many columns (rows for a chart by row), so last month's chart draws this month's rows, and one of part of a table, or of whole columns or rows, keeps its range. A chart whose table now has other columns, or whose range is empty now, is removed. The context line lists the removed charts first, then which were re-pointed and which kept their range. One undo step, which brings the removed charts back. Not offered for `.xlsx`, which holds several sheets |1717 | Replace spreadsheet | Opens the file instead, as File > Open and the command line do, asking first when there are unsaved changes |1818 1919 | Format | Import | Download |@@ -217,14 +217,24 @@ func TestImportLocation(t *testing.T) {
217217 t.Errorf("tabs after undo %q", l)218218 }219219 220+ // A chart of the one-cell table in A1 doesn't fit the two columns221+ // replacing it: it goes, and undo brings it back.222+ s.keys("<ctrl+home>")223+ insertChart(s)224+ s.keys("<enter>", "<esc>")225+ s.waitFor("No numbers to chart")220226 openImportPicker(s)221227 s.keys("tsv", "<enter>")222228 s.waitFor("Import location")223229 s.keys("current", "<enter>")224- s.waitFor("Imported budget.tsv into Sheet1 (2 rows)")230+ s.waitFor("Imported budget.tsv into Sheet1 (2 rows); removed charts: Chart 1 (A1)")225231 s.waitForBar("A1", "Rent")232+ if strings.Contains(s.screen(), "No numbers to chart") {233+ t.Errorf("the chart stayed:\n%s", s.screen())234+ }226235 s.keys("<ctrl+z>")227236 s.waitFor("Undid: import budget.tsv")237+ s.waitFor("No numbers to chart")228238 s.keys("<ctrl+home>")229239 s.waitForBar("A1", "Notes")230240 }@@ -75,8 +75,9 @@ func (w *Workbook) InsertBook(src *Workbook, at int, label string) (ImportResult
7575 // ReplaceSheet puts s, the sheet of an imported workbook, in the place of7676 // dst, as one undo step labelled label: it takes dst's name and position,7777 // so formulas reading dst read it, and named ranges on dst move to it.78-// dst's charts stay, drawing s's data (see keepCharts), and it returns79-// what became of each. dst's cells and the rest go; undo brings them back.78+// dst's charts that fit s's data stay, drawing it, and the rest go (see79+// keepCharts); it returns what became of each. dst's cells and the rest80+// go; undo brings them back, charts too.8081 func (w *Workbook) ReplaceSheet(dst, s *Sheet, label string) ([]ChartFate, error) {8182 switch {8283 case !dst.live || dst.wb != w:@@ -108,15 +109,20 @@ func (w *Workbook) ReplaceSheet(dst, s *Sheet, label string) ([]ChartFate, error
108109 type ChartFate struct {109110 Name string // the chart's title, or Chart 1, Chart 2 by position110111 Was, Now Rect // the range it drew, and draws111- Empty bool // the new data has nothing in Now112+ // Removed is set when the range no longer fits the new data: it113+ // holds nothing now, or it was a whole table and the new sheet's114+ // table there has other series.115+ Removed bool112116 }113117 114-// keepCharts gives s, replacing old, old's charts. A chart that drew a115-// whole block of old's data is re-pointed to the block s has at the same116-// corner when it has as many series (columns, or rows for a chart by117-// row), so a chart of last month's table draws this month's, however118-// many rows it has. Any other chart keeps its range, which may now hold119-// something else or nothing; ChartFate says which.118+// keepCharts gives s, replacing old, old's charts that fit its data. A119+// chart that drew a whole block of old's data is re-pointed to the block120+// s has at the same corner when it has as many series (columns, or rows121+// for a chart by row), so a chart of last month's table draws this122+// month's, however many rows it has; when s's block there has other123+// series the chart goes. A chart of whole columns or rows, or of part of124+// a block, keeps its range. A chart whose range holds nothing now goes125+// too. ChartFate says which.120126 func (s *Sheet) keepCharts(old *Sheet) []ChartFate {121127 fates := make([]ChartFate, 0, len(old.charts))122128 for i, c := range old.charts {@@ -125,30 +131,36 @@ func (s *Sheet) keepCharts(old *Sheet) []ChartFate {
125131 name = fmt.Sprintf("Chart %d", i+1)126132 }127133 was := c.Data128- c.Data = s.repoint(old, was, c.ByRow)129- _, filled := s.FilledBounds(c.Data)130- fates = append(fates, ChartFate{Name: name, Was: was, Now: c.Data, Empty: !filled})131- s.charts = append(s.charts, c)134+ now, fits := s.repoint(old, was, c.ByRow)135+ _, filled := s.FilledBounds(now)136+ f := ChartFate{Name: name, Was: was, Now: now, Removed: !fits || !filled}137+ fates = append(fates, f)138+ if !f.Removed {139+ c.Data = now140+ s.charts = append(s.charts, c)141+ }132142 }133143 return fates134144 }135145 136146 // repoint is the range on s a chart drawing d on old draws: s's block of137147 // data at d's corner when d was old's whole block there and the two have138-// as many series, else d. Whole columns or rows read any data as is.139-func (s *Sheet) repoint(old *Sheet, d Rect, byRow bool) Rect {148+// as many series, else d. Whole columns or rows, and part of a block,149+// read any data as is. It reports false when d was a whole block and s150+// has none like it there.151+func (s *Sheet) repoint(old *Sheet, d Rect, byRow bool) (Rect, bool) {140152 if d.AllRows() || d.AllCols() || old.Region(d.From) != d {141- return d153+ return d, true142154 }143155 now := s.Region(d.From)144156 if now.From != d.From || s.cells.get(now.From).Blank() && now.From == now.To {145- return d157+ return d, false146158 }147159 if byRow && now.To.Row-now.From.Row != d.To.Row-d.From.Row ||148160 !byRow && now.To.Col-now.From.Col != d.To.Col-d.From.Col {149- return d161+ return d, false150162 }151- return now163+ return now, true152164 }153165 154166 // freeIn returns base, or base with a number added ("Sales 2"), so that@@ -112,9 +112,11 @@ func namesOf(w *Workbook) string {
112112 // hidePlain hides s outside the history, as a loader would.113113 func (w *Workbook) hidePlain(s *Sheet) { s.tabHidden = true }114114 115-// Replacing a sheet keeps its charts: one that drew the whole table is116-// re-pointed to the new table when it has as many columns, and the rest117-// keep their ranges, whatever they now hold.115+// Replacing a sheet keeps the charts that fit the new data: one that116+// drew the whole table is re-pointed to the new table when it has as117+// many columns and removed when it hasn't; one of part of a table keeps118+// its range; one whose range is empty now is removed. Undo brings the119+// removed ones back.118120 func TestReplaceSheetKeepsCharts(t *testing.T) {119121 w := bookOf(t, page{"Sales", map[string]string{120122 "A1": "Month", "B1": "Total", "A2": "Jan", "B2": "1", "A3": "Feb", "B3": "2",@@ -124,6 +126,7 @@ func TestReplaceSheetKeepsCharts(t *testing.T) {
124126 old.AddChart(Chart{Data: NewRect(at("A1"), at("B3")), Title: "Totals"})125127 old.AddChart(Chart{Data: NewRect(at("E1"), at("E2"))})126128 old.AddChart(Chart{Data: NewRect(at("H1"), at("H1"))})129+ old.AddChart(Chart{Data: NewRect(at("B2"), at("B3")), Title: "Part"})127130 w.ClearHistory()128131 src := bookOf(t, page{"sales", map[string]string{129132 "A1": "Month", "B1": "Total", "A2": "Jan", "B2": "4", "A3": "Feb", "B3": "5", "A4": "Mar", "B4": "6",@@ -135,8 +138,9 @@ func TestReplaceSheetKeepsCharts(t *testing.T) {
135138 }136139 want := []ChartFate{137140 {Name: "Totals", Was: NewRect(at("A1"), at("B3")), Now: NewRect(at("A1"), at("B4"))},138- {Name: "Chart 2", Was: NewRect(at("E1"), at("E2")), Now: NewRect(at("E1"), at("E2"))}, // a column more: kept139- {Name: "Chart 3", Was: NewRect(at("H1"), at("H1")), Now: NewRect(at("H1"), at("H1")), Empty: true},141+ {Name: "Chart 2", Was: NewRect(at("E1"), at("E2")), Now: NewRect(at("E1"), at("E2")), Removed: true}, // a column more142+ {Name: "Chart 3", Was: NewRect(at("H1"), at("H1")), Now: NewRect(at("H1"), at("H1")), Removed: true}, // empty143+ {Name: "Part", Was: NewRect(at("B2"), at("B3")), Now: NewRect(at("B2"), at("B3"))},140144 }141145 if len(fates) != len(want) {142146 t.Fatalf("fates %+v", fates)@@ -147,11 +151,11 @@ func TestReplaceSheetKeepsCharts(t *testing.T) {
147151 }148152 }149153 charts := w.Sheet(0).Charts()150- if len(charts) != 3 || charts[0].Data != want[0].Now || charts[0].Title != "Totals" {154+ if len(charts) != 2 || charts[0].Data != want[0].Now || charts[0].Title != "Totals" || charts[1].Title != "Part" {151155 t.Errorf("charts on the new sheet: %+v", charts)152156 }153157 w.Undo()154- if w.Sheet(0) != old || len(old.Charts()) != 3 || old.Charts()[0].Data != want[0].Was {158+ if w.Sheet(0) != old || len(old.Charts()) != 4 || old.Charts()[0].Data != want[0].Was {155159 t.Errorf("undo: charts %+v", old.Charts())156160 }157161 }@@ -121,21 +121,27 @@ func sheetList(sheets []*sheet.Sheet) string {
121121 }122122 123123 // chartFates tells what replacing a sheet did to its charts, for the124-// note: re-pointed to the new data, or kept on their range.124+// note: re-pointed to the new data, kept on their range, or removed, as125+// they no longer fit (undo brings them back).125126 func chartFates(fates []sheet.ChartFate) string {126- if len(fates) == 0 {127- return ""128- }129- parts := make([]string, len(fates))130- for i, f := range fates {127+ var kept, removed []string128+ for _, f := range fates {131129 switch {130+ case f.Removed:131+ removed = append(removed, f.Name+" ("+f.Was.String()+")")132132 case f.Now != f.Was:133- parts[i] = f.Name + " re-pointed to " + f.Now.String()134- case f.Empty:135- parts[i] = f.Name + " kept on " + f.Now.String() + ", empty now"133+ kept = append(kept, f.Name+" re-pointed to "+f.Now.String())136134 default:137- parts[i] = f.Name + " kept on " + f.Now.String()135+ kept = append(kept, f.Name+" kept on "+f.Now.String())138136 }139137 }140- return "; charts: " + strings.Join(parts, ", ")138+ // Removals first: they matter more when the note is cut short.139+ out := ""140+ if len(removed) > 0 {141+ out += "; removed charts: " + strings.Join(removed, ", ")142+ }143+ if len(kept) > 0 {144+ out += "; charts: " + strings.Join(kept, ", ")145+ }146+ return out141147 }@@ -76,24 +76,25 @@ func TestImportLocation(t *testing.T) {
7676 }7777 press(t, m, "<ctrl+z>")7878 79- // Replace current sheet keeps the sheet's name, place and charts,80- // and says what became of the charts.79+ // Replace current sheet keeps the sheet's name, place and the charts80+ // that fit the new data, removes the rest, and says which.8181 press(t, m, "<ctrl+pgup>")8282 m.sheet.AddChart(sheet.Chart{Data: sheet.NewRect(addr("C1"), addr("C2")), Title: "Totals"})83+ m.sheet.AddChart(sheet.Chart{Data: sheet.NewRect(addr("A1"), addr("B2")), Title: "Regions"})8384 writeFile(t, "sales.csv", "Region,Total\nSouth,50\n")8485 importInto(t, m, "sales.csv", "Replace current sheet")8586 if sheetNames(m) != "Sheet1,sales" || m.sheet.Name() != "Sheet1" || input(m, "A2") != "South" || input(m, "A1") != "Region" {8687 t.Fatalf("replaced: %s on %s, A2 %q", sheetNames(m), m.sheet.Name(), input(m, "A2"))8788 }88- if line(m, contextLine) != "Imported sales.csv into Sheet1 (2 rows); charts: Totals kept on C1:C2, empty now" {89- t.Errorf("context %q", line(m, contextLine))89+ if got := line(m, contextLine); !strings.HasPrefix(got, "Imported sales.csv into Sheet1 (2 rows); removed charts: Totals (C1:C2); charts:") {90+ t.Errorf("context %q", got)9091 }91- if len(m.sheet.Charts()) != 1 {92- t.Errorf("charts after replacing: %+v", m.sheet.Charts())92+ if c := m.sheet.Charts(); len(c) != 1 || c[0].Title != "Regions" {93+ t.Errorf("charts after replacing: %+v", c)9394 }9495 press(t, m, "<ctrl+z>")95- if m.sheet.Name() != "Sheet1" || input(m, "A1") != "=sales!B2*2" {96- t.Errorf("undo replace: on %s, A1 %q", m.sheet.Name(), input(m, "A1"))96+ if m.sheet.Name() != "Sheet1" || input(m, "A1") != "=sales!B2*2" || len(m.sheet.Charts()) != 2 {97+ t.Errorf("undo replace: on %s, A1 %q, %d charts", m.sheet.Name(), input(m, "A1"), len(m.sheet.Charts()))9798 }9899 }99100