TkinterEP / ttkwidgets

A collection of widgets for Tkinter's ttk extensions by various authors
GNU General Public License v3.0
138 stars 28 forks source link

Merge Tooltip from tkinterpp #39

Open RedFantom opened 4 years ago

RedFantom commented 4 years ago

Widget: CreateToolTip File: tkinterpp/tooltip.py Functionality: Create a tooltip similar to the Balloon widget to provide a tooltip when hovering over a widget.

TODO:

sbordeyne commented 4 years ago

I've glanced over your balloon implementation, and there is a couple of things I don't agree on

Overall, I like:

Proposed changes :

Thoughts ?

RedFantom commented 4 years ago

Actually I was making changes while you posted this. I'll elaborate on them. I created three new options. offset to make the offset from the cursor position an option, showheader to allow hiding the header when desired, and static to allow setting the position relative to the master widget rather than to the cursor, which I quite liked about your tooltip implementation.

I also think ToolTip would be a better name than Balloon, all things considered.

Now, to address your points of critique:

The balloon is too big, specifically around the height

This is part of the padding on the widgets. I am not opposed to making this an option with a default smaller value than it currently is.

I don't think the graphic is all that necessary to be honest. I think it'd be best to leave it out

It is an option now, whether it should be on by default is something to be considered.

I'd have to fire up PyQt to check, but I seem to recall that its default tooltip color is less yellow. This is minor though.

The widget was designed with Windows in mind, and this was the default colour for Win32 tooltips, or something like that. I wouldn't necessarily mind making it less yellow, but for people on crappy screens that might make it look white (actually a screen I have to use on the weekends has that tendency), so I'm not sure whether making it less yellow would actually be a good idea.

The balloon is a widget itself, having a widget builder like the CreateTooltip class has its pros

Actually Balloon is a widget that can also just be used as a builder. Having it also act as a widget allows changing options afterwards, which I personally think is important. You can check out the original application for which I designed it if you want to take a look.

Okay, now on to the proposed changes:

tooltip args are passed as either a string, or a tuple into the base widget class with the tooltip= kwarg

This is out of scope of changing a widget, in my opinion. I think this is a good idea, but it has to be implemented at a very basic level. Either all ttkwidgets have to have explicit support for this by using a mixin of some kind, or the base class of all Tk widgets must be modified with a mixin during runtime. Let's first do the widget itself and then see about adding an option to create a widget more easily.

We keep text, background, width (line width in character), and those are passed onto the tooltip builder class. We add a max_width arg for the max width of the window in px

Personally, I find it easier to think in terms of pixels rather than in terms of characters when it comes to the width of widgets, though I can see how doing this in terms of characters could benefit, for example, HiDPI screen users with scaling. The logic for what you want to do might be a bit resource intensive though. As of right now, the Canvas and its children are recreated every time show is executed as the options might have changed in the meantime, and iteratively checking whether the length of the text exceeds the maximum width of the widget might cause it to slow down. The creation of the widgets could be moved to a separate function that will re-create them after configure has been called, making it faster to show. What do you think? Edit JIT: Scratch that, that is not a viable option as that would require the Toplevel to exist the whole time, as the widgets are its children. That would mean adding a loop that might be slow to the show() function. It is worth checking how slow that would be.

sbordeyne commented 4 years ago

I agree with most of what you said.

Personally, I find it easier to think in terms of pixels rather than in terms of characters when it comes to the width of widgets, though I can see how doing this in terms of characters could benefit, for example, HiDPI screen users with scaling

I think in this instance, characters make more sense. textwrap.wrap() expects a width in characters, plus wrapping at pixel values would just be weird.

The logic for what you want to do might be a bit resource intensive though. As of right now, the Canvas and its children are recreated every time show is executed as the options might have changed in the meantime,

For the canvas being recreated every time, we can actually cache the tooltips. Reminder : I did make functions to move/copy widgets, so the toplevel having to exist the whole time is a non-issue as there is a workaround. Moving the widget to its parent, not packing it, and moving it back after the toplevel is recreated.

and iteratively checking whether the length of the text exceeds the maximum width of the widget might cause it to slow down.

It's not necessary, as I made a function to get the average character width for a font. If we want something clean, we can just use tkinter.font.Font().measure() to have an exact measurement and it's quite fast, especially if the font is cached

RedFantom commented 4 years ago

With regards to this commit: Please just use a single branch for a single purpose. Like I said before, I'd like to implement something like the tooltips you imagined, but not in this branch. This branch and issue are just for merging/improving the tooltips that are already available.

But as far as I can tell, your copying function actually relies on recreating the widgets using the information from the previous widgets, which is no faster than recreating them using known values. This is not so much about how much code it costs, as how much time. Tkinter is already a bit slow, it will become even more so if all widgets have tooltips and all the tooltips require loops in the function that makes them show up.

Wrapping the text before creating the label is the best choice, I think. That way, only when the text and/or width are changed does the text have to be wrapped again. But wrapping by Tkinter is always the fastest way to do it, and the benefits of wrapping by character length seem small to me, non-existent even if it does not provide a benefit in environments with scaling. After all: The library will be used by the developer, and pixels and being able to use a number of pixels might even be beneficial if you want users to be able to change the font size or something like that.

I will try to check whether there is an advantage when using DPI scaling on monitors, but until then, I am not sure what road is the right one.

sbordeyne commented 4 years ago

With regards to this commit: Please just use a single branch for a single purpose. Like I said before, I'd like to implement something like the tooltips you imagined, but not in this branch. This branch and issue are just for merging/improving the tooltips that are already available.

Yeah sorry. This util actually went along the lines of using this in the tooltip class (hence why it's in that branch). I've forgotten how exactly I was going to use it (silly me, but I'm sure it'll come back to me). Something along the lines of wrapping or setting the toplevel width to the proper size or something. I can remove it if you want.

But as far as I can tell, your copying function actually relies on recreating the widgets using the information from the previous widgets, which is no faster than recreating them using known values. This is not so much about how much code it costs, as how much time. Tkinter is already a bit slow, it will become even more so if all widgets have tooltips and all the tooltips require loops in the function that makes them show up.

True, I hadn't thought about it this way.

Wrapping the text before creating the label is the best choice, I think. That way, only when the text and/or width are changed does the text have to be wrapped again. But wrapping by Tkinter is always the fastest way to do it, and the benefits of wrapping by character length seem small to me, non-existent even if it does not provide a benefit in environments with scaling. After all: The library will be used by the developer, and pixels and being able to use a number of pixels might even be beneficial if you want users to be able to change the font size or something like that.

We can use tkinter's wrapping for now, but in the end, one of the biggest drawback of tkinter is how hard it is to make a clean responsive app, with widgets that place and size properly based on the window size. Having text wrap on character length would increase consistency, and avoid the developer some additionnal overhead code.

I'd like you to edit the OP to reflect the changes discussed here, so that we can have a clear direction as to how we should proceed moving forward. These long texts are not the best way to keep track of tasks done and to be done.

sbordeyne commented 4 years ago

So far :

WinEunuuchs2Unix commented 3 years ago

@RedFantom I have the ttkwidgets PPA repository installed under Ubuntu. I use CheckBoxTreeview successfully. Just now I copied the code for tooltips.py into my directory but when I try to import it get:

Traceback (most recent call last):
  File "./m", line 29, in <module>
    import mserve  # Script loaded as module for .pyc
  File "/home/rick/python/mserve.py", line 152, in <module>
    import tooltips
  File "/home/rick/python/tooltips.py", line 23, in <module>
    from ttkwidgets.hook import hook_ttk_widgets, generate_hook_name, is_hooked
ImportError: No module named hook

Can you tell me what I need to do?

Thanks,

rdbende commented 3 years ago

I don't think this issue belongs to #39, but if I know well, the problem is that the PPA repository is not up to date, and doesn't yet include the hook module. Try to install it with pip.

WinEunuuchs2Unix commented 3 years ago

@rdbende My python script already has 30 apt get requirements plus adding PPA for tkkwidgets. I'd hate to add a requirement to install pip as well. To compound the suggestion I've never used pip or pip3 and everything has been through sudo apt install for dependencies. Perhaps it would be simpler to create new request to update the PPA?

rdbende commented 3 years ago

Then yes.

RedFantom commented 3 years ago

It's kind of complicated to move this to a separate issue now, so I'll just leave it as-is, but this would indeed have been more appropriate in a separate issue.

@rdbende is correct, you need to also have the hook.py module in order for the new tooltips to work. There's probably the Balloon widget in the PPA-version of the package though. Do note that you will have to comply with the terms of GNU GPLv3 if you copy any code files into your project.

Perhaps it would be simpler to create new request to update the PPA?

You can ask, but the PPA is maintained by @j4321 , and she hasn't been very active lately. Maybe she can update it, but I'm not sure. If you want to use Python, it's a good idea to get into the habit of using pip anyway. If your 30 apt-get dependencies are all Python packages, you can probably install them with pip as well, which is the preferred method.

WinEunuuchs2Unix commented 3 years ago

@RedFantom Last night I implemented tooltips with native tkinter code as published here: http://www.voidspace.org.uk/python/weblog/arch_d7_2006_07_01.shtml#e387 Then I made a small modification from yellow balloon to reversed foreground/background colors of the widget being hovered over. Now I just need to figure out how to implement hover delay using .after(1000, function_name_from_parent_class).

My apologies for not posting this question in the correct spot! Also for my ignorance of tooltips.py licensing :(

As for pip you make some good points. Another good point would be pip simplies transition to Windows, Mac and other Linux non-Debian distros who don't use apt-get.

Thank you for your time and advice.