GetSheetIndex and CopySheet bug

[shenwei356]

Hi @xuri and other contributors, I tried to copy "Sheet1" in a .xlsx file, but it reported

invalid worksheet index

Reproducible data (t.xlsx, created with this excellent package) and code:

package main

import (
	"fmt"
	"os"

	"github.com/xuri/excelize/v2"
)

func main() {
	xlsx, err := excelize.OpenFile("t.xlsx")
	checkError(err)
	defer checkError(xlsx.Close())

	fmt.Println("existed sheets:")
	sheets := xlsx.GetSheetMap()
	for idx, sheet := range sheets {
		fmt.Println("  ", idx, sheet)
	}

	index, err := xlsx.NewSheet("Sheet2")
	checkError(err)
	fmt.Printf("the index of the new sheet: %d\n", index)

	err = xlsx.CopySheet(1, index)
	if err != nil {
		checkError(fmt.Errorf("failed to copy sheet: %s", err))
	}

	checkError(xlsx.SaveAs("t.copy.xlsx"))
}

func checkError(err error) {
	if err != nil {
		fmt.Println(err)
		os.Exit(-1)
	}
}

$ go run t.go 
existed sheets:
   1 Sheet1
the index of the new sheet: 1
failed to copy sheet: invalid worksheet index
exit status 255

The version v2.7.1 (even v2.6.1) and the master branch of excelize can both reproduce this result.

After checking the source code of excelize , I found the direct bug is that GetSheetIndex returns a 0-based index, which was called in NewSheet and other methods.

In GetSheetIndex methods, the method GetSheetList was called (returned index is 0-based!!!), which should be GetSheetMap, so did other methods.

I forked this package and replaced GetSheetList with GetSheetMap; the test code above could pass, but some other tests failed with go test .. It looks like much effort needs to be taken from more experienced authors.

BTW, SetActiveSheet seems to use another type of index, where 0 is allowed. The doc is not clear to me, could you please add more details?

// SetActiveSheet provides a function to set the default active sheet of the
// workbook by a given index. Note that the active index is different from the
// ID returned by function GetSheetMap(). It should be greater than or equal to 0
// and less than the total worksheet numbers.
func (f *File) SetActiveSheet(index int) {

[xuri]

Thanks for your issue. As the documentation of the GetSheetMap function says, that returns the sheet ID and name map of the workbook, but the CopySheet function needs to copy sheet by sheet index, which was different from the sheet ID. The sheet ID was a 1-based identity used in the workbook internally, but the sheet index was a 0-based integer representing the order of the worksheet. So, you need to get the sheet index for the default worksheet Sheet1 with the GetSheetIndex function before copying sheets like this:

+from, err := xlsx.GetSheetIndex("Sheet1")
+checkError(err)
+err = xlsx.CopySheet(from, index)
-err = xlsx.CopySheet(1, index)

[shenwei356]

Thanks for the classification. I didn't notice the difference between "sheet ID" and "sheet index".