crahan / ipyfilechooser

Python file chooser widget for use in Jupyter/IPython in conjunction with ipywidgets
MIT License
103 stars 19 forks source link

Specifying width on creation seems to have no effect #56

Closed tombelpasso closed 3 years ago

tombelpasso commented 3 years ago

I've been looking for a decent file chooser for Jupyter, thanks for making one. I have pretty long paths, particularly when working on Google CoLab. The path name was getting truncated. The widget wasn't using the full width of the page, so I tried to specify a width of 100%.

fc = FileChooser(title="Source Video",layout=Layout(width='100%'))

Error message:

C:\Portable\PortableApps\Portable Python-3.8.6 x64\App\Python\lib\site-packages\ipyfilechooser\filechooser.py in init(self, path, filename, title, select_desc, change_desc, show_hidden, select_default, use_dir_icons, show_only_dirs, filter_pattern, **kwargs) 132 133 # Call VBox super class init --> 134 super().init( 135 children=[ 136 self._title,

TypeError: init() got multiple values for keyword argument 'layout'

crahan commented 3 years ago

Thanks for taking the time to provide feedback!

I currently haven't exposed the layout parameter. The reason for the error is most likely because I already supply the layout parameter with an 'auto' width value using layout=Layout(width='auto') when I call the VBox __init__() function. So specifying it a second time will probably cause that error you're seeing.

That said, I'm not sure that being able to specify a layout for the outer most VBox object would fix your problem as the GridBox element (which contains the folder contents, path selector, and filename textfield) is set to a 500px width and all other UI elements have an 'auto' width which wraps them around that size.

I'll see if I can come up with a suitable solution in a upcoming release. Would the ability to specify a width value (e.g. "100%", "auto", "650px") suffice instead of providing a full Layout object as a parameter? I definitely agree that the width should be configurable though, so I want to find a good solution to allow for that.

tombelpasso commented 3 years ago

Right now I guess width is most important.

I noticed that on line 106 of ipyfilechooser.py is the only place where the width isn't set to 'auto':

            width='500px',

Maybe that is forcing the size of widget?

crahan commented 3 years ago

That's correct and that would be the value I'd suggest making configurable via a parameter. The default would be '500px', but you would be able to override it with whatever width you prefer.

tombelpasso commented 3 years ago

I would recommend changing that line, 106, so the width is also 'auto'

I did that locally and now the browser expands and contracts with the window width as most people would expect. I used your browser to find the location of the file, so I could edit it.

By the way I tried overriding the width using the following commands but neither made a difference.

fc = FileChooser(title="Source Video",width='auto') fc.layout.width = '100%'

Only changing line 106 made a difference.

crahan commented 3 years ago

That's correct. My suggestion is to make line 106 configurable with 500px as the default. There's quite a few people who use the package with 500px as the default and I don't want to change that behavior.

I do however want to make it configurable by allowing the user to specify 'auto', '100%', or any other width string as a parameter. It won't work with the current version (which is why your examples did not yield the expected results), but I will update in the next release so width can be used as a valid parameter.

crahan commented 3 years ago

Actually, after some quick tests I'll make line 106 'auto' as you suggested, but change layout=Layout(width='auto') on line 140 to layout=Layout(width='500px') instead and make sure you can specify your own layout to override that default '500px' value. That keeps the default width as-is for existing users, but allows anyone who wants to change the width to do so by specifying their own layout parameter on the FileChooser object.

Here's how it would work without the layout parameter:

Screen Shot 2021-08-20 at 03 04 11

And here is what would happen when you specify your own layout value, in this case Layout(width='100%'):

Screen Shot 2021-08-20 at 03 03 12

I've added a new branch named width_and_fixes that has the updated code:

https://github.com/crahan/ipyfilechooser/blob/1a2017cd2752d21594f2b7ffba5efb261dd163b6/ipyfilechooser/filechooser.py#L25-L28

https://github.com/crahan/ipyfilechooser/blob/1a2017cd2752d21594f2b7ffba5efb261dd163b6/ipyfilechooser/filechooser.py#L134-L143

I need to implement a few other minor fixes first before I can push out a new release though, but the width fix will definitely be included.

tombelpasso commented 3 years ago

I continued testing and found the putting width value in the creation command has no effect.

fc = FileChooser(title="Source Video",width='800px')

Only setting the value, before displaying the chooser changes the width.

fc.layout.width = '400px'

Once the chooser is displayed, changing the width attribute has no effect.

I would recommend having either layout or width as keywords to the init method with default value like '500px', then the user can override value during creation. Although I would also recommend that the default be a percentage like '50%' that produces similar size. This would allow the chooser be resized by making the window larger to see the values in the fields.

Unless you really have a good reason for the '500px' value. It would be a mistake to maintain just for backwards compatibility, since it's absolute value impedes functionality of your otherwise very useful code. If someone really complains, just tell them how to set the width.

Think of your future users which hopefully be more numerous who will expect auto width.

Thank you for your effort.

If you need help with documentation, I can help.

crahan commented 3 years ago

I continued testing and found the putting width value in the creation command has no effect.

That is correct. The parameter to change the width is now layout which expects a Layout object type. Can you please confirm that you have tested with the code in the new width_and_fixes branch?

Once the chooser is displayed, changing the width attribute has no effect.

I did not run into that issue with the updated code. Once a FileChooser object has been created I was able to change the width on the fly. Please see the demo video below.

https://user-images.githubusercontent.com/172588/130201387-ad59abea-8e40-404f-8ded-de5883ed249b.mov

I would recommend having either layout or width as keywords to the init method with default value like '500px', then the user can override value during creation.

The updated code in the width_and_fixes branch now supports using the layout parameter to provide a Layout object to set the width. See the sample code below. '100%' and 'auto' will both make the FileChooser object take up the full width of the screen (as also shown in the demo video above).

from ipywidgets import Layout

fc = FileChooser(title="Source Video", layout=Layout(width='100%'))

Although I would also recommend that the default be a percentage like '50%' that produces similar size. This would allow the chooser be resized by making the window larger to see the values in the fields.

A percentage changes the size of the widget as the window is resized and might not be expected behavior. It might result in roughly the same width in some cases, but it will vary per user and project.

Unless you really have a good reason for the '500px' value. It would be a mistake to maintain just for backwards compatibility, since it's absolute value impedes functionality of your otherwise very useful code. If someone really complains, just tell them how to set the width.

I fully agree that the width of the widget should be configurable and the updated code in the width_and_fixes branch now supports that. However, as this is the first issue related to the width of the widget it appears that the '500px' width default works for most users and is a suitable default value, at least for the next release. I'll give it some additional thought, so this might still change in future releases.

I will keep this issue open until the width_and_fixes branch is merged and a new release is pushed to pypi.org.

crahan commented 3 years ago

v0.5.0 has now been published to PyPi. Conda-forge feedstock release will follow shortly. Thanks again for reporting this issue and helping to make the project better!