Akascape / CTkListbox

A simple listbox for customtkinter (extenstion/add-on)
MIT License
130 stars 14 forks source link

Problem when deleting an option #33

Closed Vicktorus88 closed 5 months ago

Vicktorus88 commented 6 months ago

When i delete an option and i click on the folowing options, the option after my selection is selected. So if i delete index 3 and then click on index 4, index 5 is selected.

BTW, thank you for your widgets. GOOD JOB !

import customtkinter
from CTkListbox import *

def show_value(selected_option):
    print(selected_option)

root = customtkinter.CTk()

listbox = CTkListbox(root, command=show_value)
listbox.pack(fill="both", expand=True, padx=10, pady=10)

listbox.insert(0, "Option 0")
listbox.insert(1, "Option 1")
listbox.insert(2, "Option 2")
listbox.insert(3, "Option 3")
listbox.insert(4, "Option 4")
listbox.insert(5, "Option 5")
listbox.insert(6, "Option 6")
listbox.insert(7, "Option 7")
listbox.insert("END", "Option 8")

def delete():
    listbox.delete(3)

button = customtkinter.CTkButton(root, text="Delete", command=delete)
button.pack(fill="both", expand=True, padx=10, pady=10)

root.mainloop()
P3rdigas commented 6 months ago

I seem to found the reason for the issue! Basically when you insert an option, the function select will be assign with a giving index for that option (when inserting the option with index for 4, his select function is assign with the index 4 too), however when you delete an option it doesn't update the index for the select function (meaning that when you delete the option with index 3 it will delete it and the option 4 will change his index to 3, and for the rest of the options, but the index inside the select function for the option 4 is not updated, stays as 4, which results in selecting the option 5 that currently has the index 4).

I hope this explanation can be clear to you!

I will try to work on an solution and update you if i achieve it.

Vicktorus88 commented 6 months ago

I fixed it modifying class CTkListbox. I put in comment line 78 and juste use the index instead of the value to assigne selected_button. I'm not sure in witch case index is not an int(). i feel that the line in comments get the object button by name instead of the index... something like this...

    def select(self, index):
        """ select the option """
        for options in self.buttons.values():
            options.configure(fg_color=self.button_fg_color)

        if isinstance(index, int):
            #selected_button = list(self.buttons.values())[index]
            selected_button = self.buttons[index]
        else:
            selected_button = self.buttons[index]
Vicktorus88 commented 6 months ago

it is a temporary fix. i would prefer to use your repo for long time maintenance.

P3rdigas commented 6 months ago

This was my approach:

def delete(self, index, last=None):
        """delete options from the listbox"""

        def recalculate_select(self, index):
            for i in range(int(index), len(self.buttons)):
                current_key = list(self.buttons.keys())[i]
                self.buttons[current_key].configure(
                    command=lambda num=i: self.select(num)
                )

        if str(index).lower() == "all":
            for i in self.buttons:
                self.buttons[i].destroy()
            self.buttons = {}
            self.end_num = 0
            return

        if str(index).lower() == "end":
            index = f"END{self.end_num}"
            self.end_num -= 1
        else:
            if int(index) == len(self.buttons):
                index = len(self.buttons) - 1
            if int(index) > len(self.buttons):
                return
            if not last:
                index = list(self.buttons.keys())[int(index)]

        if last:
            if str(last).lower() == "end":
                last = len(self.buttons) - 1
            elif int(last) >= len(self.buttons):
                last = len(self.buttons) - 1

            deleted_list = []
            for i in range(int(index), int(last) + 1):
                list(self.buttons.values())[i].destroy()
                deleted_list.append(list(self.buttons.keys())[i])
                self.update()
            for i in deleted_list:
                del self.buttons[i]

            recalculate_select(self, index)
        else:
            self.buttons[index].destroy()
            del self.buttons[index]

            recalculate_select(self, index)

Basically, after deleting the button, the following buttons should change their index in the select function.

For what i understand about the code the variable buttons is an dictionary, an strings will be possible as keys (you can see that in the example "END" is used as a the index for the 8 option), which can create others issues.

Vicktorus88 commented 6 months ago

works the first time, but same problem on next delete. I don't think this is true: "...when you delete the option with index 3 it will delete it and the option 4 will change his index to 3, and for the rest of the options..."

this is a log after deleting index 3: (as you can see index 3 doesn't exist anymore after deleting)

keys[index]0 values[index].!ctkframe.!canvas.!ctklistbox.!ctkbutton option name:Option 0

keys[index]1 values[index].!ctkframe.!canvas.!ctklistbox.!ctkbutton2 option name:Option 1

keys[index]2 values[index].!ctkframe.!canvas.!ctklistbox.!ctkbutton3 option name:Option 2

keys[index]4 values[index].!ctkframe.!canvas.!ctklistbox.!ctkbutton5 option name:Option 4

keys[index]5 values[index].!ctkframe.!canvas.!ctklistbox.!ctkbutton6 option name:Option 5

This is why i think, the problem is in the select method at line: 77 selected_button = list(self.buttons.values())[index]

i don't understand why it select the button by values()[index]. instead of buttons[index]. Works perfectly for me using buttons[index]

Vicktorus88 commented 6 months ago

Ii have replaced your recalculate_select by this new function that reassign indexes in good order to all buttons. Feel free to adapte it as you wish if you wan to use it.

 def reassign_ButtonsKeys(self):
            i = 0
            tempdic ={}
            for key, value in list(self.buttons.items()):
                tempdic[i] = self.buttons[key]
                tempdic[i].configure(
                command=lambda num=i: self.select(num)
                )
                i = i + 1
            self.buttons = tempdic.copy()
P3rdigas commented 6 months ago

I think your solution solve the most issue. My problem is understanding the logic behind the word "end" and how the keys work in this library. Maybe if that was improved this issue could be easier to fix.

Vicktorus88 commented 6 months ago

Yep, i don't understand when and how this "END" concat with the last options is useful. Also, i still don't understand line 76 to 79. Why making a difference when the key is a int() or a string(). self.buttons[index] works fine in both situations. i spend 2 hours trying to understand why...

P3rdigas commented 6 months ago

I don't understand the need to convert a dict to a list. I'm not an expert in python, maybe that approach solved some issues in the early days. But it the current code, when the key is an int, those lines will convert the dict to a list and once the options were created with an index and the list no longer contains one element, the next button will be selected. I'll try to change the class to work with a list in all places and hope to fix this issue

P3rdigas commented 6 months ago

I did an implementation with lists only. if you don't want to add new option to an index that already exist this implementation should work fine. I still have some issues in that regard but i think i have a solution and others improvements to make. If you want to delete or insert elements to the end of the list, this should be working fine. Feel free to use, change or if you encounter issues let me know! Later, if i manage to fix the issues and if i could make some improvements i will do a Pull Request.

The code:

"""
Custom ListBox for customtkinter
Author: Akash Bora
"""

import customtkinter

class CTkListbox(customtkinter.CTkScrollableFrame):
    def __init__(
        self,
        master: any,
        height: int = 100,
        width: int = 200,
        hightlight_color: str = "default",
        fg_color: str = "transparent",
        bg_color: str = None,
        text_color: str = "default",
        select_color: str = "default",
        hover_color: str = "default",
        button_fg_color: str = "default",
        border_width: int = 3,
        font: tuple = "default",
        multiple_selection: bool = False,
        listvariable=None,
        hover: bool = True,
        command=None,
        justify="left",
        **kwargs,
    ):
        super().__init__(
            master,
            width=width,
            height=height,
            fg_color=fg_color,
            border_width=border_width,
            **kwargs,
        )
        self._scrollbar.grid_configure(padx=(0, border_width + 4))
        self._scrollbar.configure(width=12)

        if bg_color:
            super().configure(bg_color=bg_color)

        self.select_color = (
            customtkinter.ThemeManager.theme["CTkButton"]["fg_color"]
            if select_color == "default"
            else select_color
        )
        self.text_color = (
            customtkinter.ThemeManager.theme["CTkButton"]["text_color"]
            if text_color == "default"
            else text_color
        )
        self.hover_color = (
            customtkinter.ThemeManager.theme["CTkButton"]["hover_color"]
            if hover_color == "default"
            else hover_color
        )
        self.font = (
            (customtkinter.ThemeManager.theme["CTkFont"]["family"], 13)
            if font == "default"
            else font
        )
        self.button_fg_color = (
            "transparent" if button_fg_color == "default" else button_fg_color
        )

        if justify == "left":
            self.justify = "w"
        elif justify == "right":
            self.justify = "e"
        else:
            self.justify = "c"
        # self.buttons = {}
        self.buttons = []
        self.command = command
        self.multiple = multiple_selection
        self.selected = None
        self.hover = hover
        # self.end_num = 0
        self.selections = []
        self.selected_index = 0

        if listvariable:
            self.listvariable = listvariable
            self.listvariable.trace_add("write", lambda a, b, c: self.update_listvar())
            self.update_listvar()

        super().bind("<Destroy>", lambda e: self.unbind_all("<Configure>"))

    def update_listvar(self):
        values = list(eval(self.listvariable.get()))
        self.delete("all")
        for i in values:
            self.insert(option=i)

    def select(self, index):
        """select the option"""
        for option in self.buttons:
            option.configure(fg_color=self.button_fg_color)

        selected_button = self.buttons[index]

        if self.multiple:
            if selected_button in self.selections:
                self.selections.remove(selected_button)
                selected_button.configure(fg_color=self.button_fg_color, hover=False)
                self.after(100, lambda: selected_button.configure(hover=self.hover))
            else:
                self.selections.append(selected_button)
            for i in self.selections:
                i.configure(fg_color=self.select_color, hover=False)
                self.after(100, lambda button=i: button.configure(hover=self.hover))
        else:
            self.selected = selected_button
            selected_button.configure(fg_color=self.select_color, hover=False)
            self.after(100, lambda: selected_button.configure(hover=self.hover))

        if self.command:
            self.command(self.get())

        self.event_generate("<<ListboxSelect>>")

    def activate(self, index):
        if str(index).lower() == "all":
            if self.multiple:
                for i in self.buttons:
                    self.select(i)
            return

        selected = self.buttons[index]
        self.select(selected)

    def curselection(self):
        index = 0
        if self.multiple:
            indexes = []
            for i in self.buttons:
                if i in self.selections:
                    indexes.append(index)
                index += 1
            return tuple(indexes)

        else:
            for i in self.buttons:
                if i == self.selected:
                    return index
                else:
                    index += 1

    def bind(self, key, func):
        super().bind_all(key, lambda e: func(self.get()), add="+")

    def deselect(self, index):
        if not self.multiple:
            self.selected.configure(fg_color=self.button_fg_color)
            self.selected = None
            return
        if self.buttons[index] in self.selections:
            self.selections.remove(self.buttons[index])
            self.buttons[index].configure(fg_color=self.button_fg_color)

    def deactivate(self, index):
        if str(index).lower() == "all":
            for i in self.buttons:
                self.deselect(i)
            return

        selected = self.buttons[index]
        self.deselect(selected)

    def insert(self, option, index: int = -1, **args):
        """add new option in the listbox"""

        button = customtkinter.CTkButton(
            self,
            text=option,
            fg_color=self.button_fg_color,
            anchor=self.justify,
            text_color=self.text_color,
            font=self.font,
            hover_color=self.hover_color,
            **args,
        )

        if 0 <= index < len(self.buttons):
            self.buttons[index].destroy()

        if index < 0:
            self.buttons.append(button)
            index = self.buttons.index(button)
        else:
            self.buttons.insert(index, button)

        self.buttons[index].configure(command=lambda num=index: self.select(num))
        self.buttons[index].pack(padx=0, pady=(0, 5), fill="x", expand=True)
        self.update()

        if self.multiple:
            self.buttons[index].bind(
                "<Shift-1>", lambda e: self.select_multiple(self.buttons[index])
            )

        return self.buttons[index]

    def select_multiple(self, button):
        selections = self.buttons
        if len(self.selections) > 0:
            last = selections.index(self.selections[-1])
            to = selections.index(button)

            if last < to:
                for i in range(last + 1, to + 1):
                    if self.buttons[i] not in self.selections:
                        self.select(i)
            else:
                for i in range(to, last):
                    if self.buttons[i] not in self.selections:
                        self.select(i)

    def delete(self, index, last=None):
        """delete options from the listbox"""

        def recalculate_index(i):
            for new_index in range(i, len(self.buttons)):
                self.buttons[new_index].configure(
                    command=lambda num=new_index: self.select(num)
                )

        if str(index).lower() == "all":
            for i in self.buttons:
                self.buttons[i].destroy()
            self.buttons = []
            return

        if int(index) < 0 or int(index) >= len(self.buttons):
            return

        if last:
            if str(last).lower() == "end":
                last = len(self.buttons) - 1
            elif (
                int(last) <= int(index)
                or int(last) < 0
                or int(last) >= len(self.buttons)
            ):
                return

            deleted_list = []
            for i in range(int(index), int(last) + 1):
                self.buttons[i].destroy()
                deleted_list.append(i)
                self.update()

            deleted_list.sort(reverse=True)
            for i in deleted_list:
                del self.buttons[i]

            recalculate_index(index)
        else:
            self.buttons[index].destroy()
            del self.buttons[index]

            recalculate_index(index)

    def size(self):
        """return total number of items in the listbox"""
        return len(self.buttons)

    def get(self, index=None):
        """get the selected value"""
        if index is not None:
            if str(index).lower() == "all":
                return list(item.cget("text") for item in self.buttons)
            else:
                return self.buttons[index].cget("text")
        else:
            if self.multiple:
                return (
                    [x.cget("text") for x in self.selections]
                    if len(self.selections) > 0
                    else None
                )
            else:
                return self.selected.cget("text") if self.selected is not None else None

    def configure(self, **kwargs):
        """configurable options of the listbox"""

        if "hover_color" in kwargs:
            self.hover_color = kwargs.pop("hover_color")
            for i in self.buttons.values():
                i.configure(hover_color=self.hover_color)
        if "button_fg_color" in kwargs:
            self.button_fg_color = kwargs.pop("button_fg_color")
            for i in self.buttons.values():
                i.configure(fg_color=self.button_fg_color)
        if "highlight_color" in kwargs:
            self.select_color = kwargs.pop("highlight_color")
            if self.selected:
                self.selected.configure(fg_color=self.select_color)
            if len(self.selections) > 0:
                for i in self.selections:
                    i.configure(fg_color=self.select_color)
        if "text_color" in kwargs:
            self.text_color = kwargs.pop("text_color")
            for i in self.buttons.values():
                i.configure(text=self.text_color)
        if "font" in kwargs:
            self.font = kwargs.pop("font")
            for i in self.buttons.values():
                i.configure(font=self.font)
        if "command" in kwargs:
            self.command = kwargs.pop("command")

        super().configure(**kwargs)

    def move_up(self, index):
        """Move the option up in the listbox"""
        if index > 0:
            previous_index = index - 1

            # Store the text of the button to be moved
            current_text = self.buttons[index].cget("text")

            # Update the text of the buttons
            self.buttons[index].configure(
                text=self.buttons[previous_index].cget("text")
            )
            self.buttons[previous_index].configure(text=current_text)

            # Clear the selection from the current option
            self.deselect(index)

            # Update the selection
            self.select(previous_index)

            # Update the scrollbar position
            if self._parent_canvas.yview() != (0.0, 1.0):
                self._parent_canvas.yview("scroll", -int(100 / 6), "units")

    def move_down(self, index):
        """Move the option down in the listbox"""
        if index < len(self.buttons) - 1:
            next_index = index + 1

            # Store the text of the button to be moved
            current_text = self.buttons[index].cget("text")

            # Update the text of the buttons
            self.buttons[index].configure(text=self.buttons[next_index].cget("text"))
            self.buttons[next_index].configure(text=current_text)

            # Clear the selection from the current option
            self.deselect(index)

            # Update the selection
            self.select(next_index)

            # Update the scrollbar position
            if self._parent_canvas.yview() != (0.0, 1.0):
                self._parent_canvas.yview("scroll", int(100 / 6), "units")
P3rdigas commented 6 months ago

This was the example i used:

import customtkinter

from CTkListbox import *

def show_value(selected_option):
    print(selected_option)

root = customtkinter.CTk()

listbox = CTkListbox(root, command=show_value, multiple_selection=False)
listbox.pack(fill="both", expand=True, padx=10, pady=10)

listbox.insert("Option 0", index=0)
listbox.insert("Option 1", index=1)
listbox.insert("Option 2", index=2)
listbox.insert("Option 3", index=3)
listbox.insert("Option 4", index=4)
listbox.insert("Option 5", index=5)
listbox.insert("Option 6", index=6)
listbox.insert("Option 7", index=7)
listbox.insert("Option 8")

# def deactivate():
#     listbox.deactivate("all")

def delete4ToEnd():
    listbox.delete(index=4, last="end")

def delete3():
    listbox.delete(index=3)

def delete4():
    listbox.delete(index=4)

def add0():
    listbox.insert(option="New Option 0", index=0)

def delete4To5():
    listbox.delete(index=4, last=5)

button = customtkinter.CTkButton(root, text="Delete Index 3", command=delete3)
button.pack(fill="both", expand=True, padx=10, pady=10)

button2 = customtkinter.CTkButton(root, text="Delete Index 4", command=delete4)
button2.pack(fill="both", expand=True, padx=10, pady=10)

button3 = customtkinter.CTkButton(
    root, text="Delete Index 4 to the End", command=delete4ToEnd
)
button3.pack(fill="both", expand=True, padx=10, pady=10)

button4 = customtkinter.CTkButton(root, text="Add at Index 0", command=add0)
button4.pack(fill="both", expand=True, padx=10, pady=10)

button5 = customtkinter.CTkButton(
    root, text="Delete Index 4 to Index 5", command=delete4To5
)
button5.pack(fill="both", expand=True, padx=10, pady=10)

# button2 = customtkinter.CTkButton(root, text="Deactivate", command=deactivate)
# button2.pack(fill="both", expand=True, padx=10, pady=10)

root.mainloop()
P3rdigas commented 6 months ago

I manage to fix the issues and made a pull request. Meanwhile, if you want to check all the changes you can check my repo https://github.com/P3rdigas/CTkListbox/tree/main.

Vicktorus88 commented 6 months ago

Dict or List: List: -better to maintain order

Dict:

Now, you have to look at the whole picture. If you store objects in it dict give you access to all elements easily. Which one is use in the overall application for this kind of transaction ? CTkList is "part of" Custom Tkinter, what is the mainstream is using ?

Personaly i prefer dict to store objects that i will need to access properties and methods.

Akascape commented 5 months ago

@P3rdigas @Vicktorus88 Sorry I was not responding. I fixed the issue. Please test the new version and lmk

P3rdigas commented 5 months ago

I think is still an issue. If is necessary to delete the last option, an error occur being KeyError: 'END1'. This probably occur, because self.end_num start at 0 and when inserting an option with index "END", the key will be "END0" and end_num will be 1. However, when deleting the key to be deleted will be "END1" and then end_num will decrease to 0. Once no value exists with the key "END1" an error occur. Changing the order of setting the key (index) and the decrease of the variable end_num should solve this issue.

P3rdigas commented 5 months ago

Despite that, i couldn't found any more issues!

Akascape commented 5 months ago

@P3rdigas i have already fixed that issue, but i uploaded the latest package before adding that.

P3rdigas commented 5 months ago

Just one last note. In the early days i tried this library, i probably did a bad approach, but i wanted to create an app where the user could import files and add them to the listbox, so when a user imported a file i inserted in the listbox with the index "END". All works fine and when the user want to delete files, the files that are seleted are deleted. But this implementation gave me an idea of a potential issue and it can occur. Let's say that exists 4 options (with key "END0", "END1", "END2" and "END3"). Now the user select the option 2, the function curselection() returns the index 2 and the delete is performed. Now let's say that the app has a button that performe a delete of the last element in the list (index="end"). So the option 3 is deleted (key "END3"), but the user perfomes that delete once again. Well here is the issue, the options with key "END2" and "END3" are deleted, however end_num is 2 (only decrease once in the delete where the index="end") so a KeyError will occur. Like i said, this probably could be fixed in the app, but keep in mind that this could happen.