Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix KDialog file open dialog and file filters #942

Merged
merged 10 commits into from
Jan 26, 2022

Conversation

w1th0utnam3
Copy link
Contributor

@w1th0utnam3 w1th0utnam3 commented Jan 17, 2022

Currently the open file picker (i.e. pickFile + saveFile == false) does not work with KDialog because it needs a --getopenfilename argument in this case.

In addition, file filters were not working. KDialog expects file filters either as explicit MIME types or in the following wildcard format: {description} (*.ext1 *.ext2 *.ext3 ...) where {description} is the string that will be shown in UI.

This PR tries to fix these issues. For the extension filters, I patch the supplied fileFilter string.

Another issue that I encountered is that you have to supply a "start directory" to KDialog if you want to supply file filters. Currently I use either fileName for this if its not empty or otherwise default to .. I'm not sure if . is the best choice. For this purpose it might be sensible to add a general startDir argument to all file picker functions to distinguish it from the fileName? A library user might specify only literally a filename as the value for fileName but for certain pickers (like KDialog) this will try to open an invalid start path (something like http://filename). Let me know what you think.

@miguelpruivo
Copy link
Owner

Thank you for submitting. @philenius would you mind reviewing this?

@philenius philenius added the desktop The issue applies to Windows, Linux or MacOS implementations. label Jan 17, 2022
@philenius
Copy link
Collaborator

@miguelpruivo, sure

@w1th0utnam3 thank you for your contribution. Support for KDialog was contributed by @mnipritom just 10 days ago in #932 and #935. (thank you, @mnipritom , btw 👍 ). Back then, @mnipritom mentioned that filtering by file type doesn't work:

Please note: I have found that, kdialog is not correctly handling file filter MIME types, where it CAN accept filters as String (eg : '*.ogg *.mp4' etc), but does not filter them unless addressing MIME types in 'video/ogg video/mp4' this format. I will be filing a bug on KDE gitlab addressing this issue. So I did not change anything regarding file filter types.

Please note that I'm not ready to merge yet. Unfortunately, I didn't test the KDialog implementation until now and I'm not happy with it due to the following reasons:

  • It contains bugs:
    • Setting a file type filter didn't work (until now, so thanks @w1th0utnam3 🚀 )
    • Selecting multiple files doesn't work because KDialog returns multiple files separated by blanks whereas qarma/zenity returns multiple files separated by vertical pipes
  • There are no unit test for the new method generateKdialogArguments().
  • The code in file_picker_linux.dart keeps inflating and more and more differences emerge between KDialog and Qarma/Zenity. That's why I want to refactor it using the Factory Method design pattern.

Please give me two more days to do the refactoring. I'll get back to you and then we can merge your contribution.

Regarding your second point:

Another issue that I encountered is that you have to supply a "start directory" to KDialog if you want to supply file filters. Currently I use either fileName for this if its not empty or otherwise default to .. I'm not sure if . is the best choice. For this purpose it might be sensible to add a general startDir argument to all file picker functions to distinguish it from the fileName? A library user might specify only literally a filename as the value for fileName but for certain pickers (like KDialog) this will try to open an invalid start path (something like http://filename). Let me know what you think.

If I understood you correctly, then you're asking for the same feature as in #859? Others requested to set the start/initial directory, too. I already provided an implementation for Linux, macOS, and Windows, but it wasn't merged because no one came up with an implementation for Android and iOS respectively. Is that correct?

@mnipritom
Copy link
Contributor

mnipritom commented Jan 18, 2022

Hi. This is exactly the type of feedback I was looking for in here #931 and thanks for providing it @philenius !

  • As for MIME types handling, since it was addressed in the first question of FAQ section

the developer can add this extra protection on their side which seems the more appropriate as you can iterate over the picked files and display a custom alert or similar.

I did not think it would be necessary to ensure strict measurements within this package over what type of files get picked, other than what is already implemented.

  • As for kdialog implementation not having expected behaviors

See these 2 screenshots as to how the kde documentation sneakily dodges this operation, indicating that it is an existing issue with the program itself.

Link to segment in documentation
Screenshot_20220118_114316

Literally the next paragraph Link to segment in documentation
Screenshot_20220118_114531

I have found a similar bug listing here, which is over 7 years old and I doubt is ever going to get addressed. Since majority of KDE community effort is going towards porting majority of the codebase over to the newer version of Qt framework.

Same goes of zenity and GTK4 porting efforts. Pretty sure qarma is a abandoned as well, not even LXQt distros are shipping it.

  • No unit testing implemented for generateKdialogArguments

That's just me being a noob, un-experienced, not going over project structure and general lack of familiarity with industry practices regarding software development. Apologies!


That all being said, with all due respect, I think documentation could use some updates reflecting the expectations of maintainers, in the form of Contributor's guide as to how one should approach contribution to the project, as it is one of the most popular 3rd party packages on pub.dev and is set to bring a lot more indie adopters like me as Ubuntu will be developing most of it's utilities using Flutter going forward.


  • Some thoughts

Since these kdialog qarma zenity or other scripting based solutions would require entirely different sets of instruction for file system operations, and even with all sorts of consideration put into this, and it still failing to target all of Linux distros and future proofing (namely Solus, which is adopting Enlightenment going forward and Tiling window manager based distros), wouldn't it be smarter to target python for file system access, which is a de-facto standard across all Linux distros, even comes with 2 separate installation python(2) python3?

This is just something I came across, brainstorming ideas as to how simplification of the filePicker would go about. I do not have the necessary technical know how, to achieve the functionality of this package going the python route. But I am very eager to learn regarding the possibilities or feasibility of this approach if anyone would like to share.

Thank again for the amazing package!

@w1th0utnam3
Copy link
Contributor Author

@mnipritom

I have found a similar bug listing here, which is over 7 years old and I doubt is ever going to get addressed. Since majority of KDE community effort is going towards porting majority of the codebase over to the newer version of Qt framework.

First of all, thanks for providing initial KDialog support. I think at the moment it is still quite useful to have this as the dialog mostly works and offers a quite nice user interface in comparison to others (looking at the barebones GTK file dialog... I really don't like programs showing this dialog even though I'm running KDE...).

wouldn't it be smarter to target python for file system access, which is a de-facto standard across all Linux distros, even comes with 2 separate installation python(2) python3?

While the concerns are definitely valid I don't think a default installation of Python provides a module for opening a file dialog, or does it? But in any case it would be nice to have some kind of fallback that would work on any Linux distribution. For example a very basic dialog using flutter? Not sure if this would be feasible in terms of integration with an app. But of course this opens a whole other can of worms like theming etc.. I guess at the moment the library would just error when you want to provide a dialog and don't have any of the three solutions installed?

See these 2 screenshots as to how the kde documentation sneakily dodges this operation, indicating that it is an existing issue with the program itself.

Yeah, I saw that as well. Further down they show an example for --getsavefilename :

kdialog --getsavefilename :label1 "C and C++ Source Files (*.cpp *.cc *.c)"

which has a working wildcard filter (not sure what the :label1 is supposed to be). So I just implemented a hacky conversion from the standard filter string to this format. (Additionally the PR allows having a --getopenfilename dialog at all, this wasn't working before).

@philenius

Please give me two more days to do the refactoring. I'll get back to you and then we can merge your contribution.

Sure. Just notify me and I can update this PR.

Selecting multiple files doesn't work because KDialog returns multiple files separated by blanks whereas qarma/zenity returns multiple files separated by vertical pipes

Ok, maybe you could take this into account while refactoring and allow the different dialog providers to also do post-processing of the picked files? Though we have to be careful with the space separator as we cannot just split at every space. I guess we would have to split such that each file starts with a / . Otherwise I think it would make more sense to just ignore the --multiple flag in the KDialog provider.

There are no unit test for the new method generateKdialogArguments().

Right, like @mnipritom mentioned this could be part of some contribution guidelines. I can help after refactoring if you want.

If I understood you correctly, then you're asking for the same feature as in #859? Others requested to set the start/initial directory, too. I already provided an implementation for Linux, macOS, and Windows, but it wasn't merged because no one came up with an implementation for Android and iOS respectively. Is that correct?

Ah yes, sorry I didn't search for this before.

@w1th0utnam3
Copy link
Contributor Author

And thanks for the detailed comments from both of you!

@mnipritom
Copy link
Contributor

mnipritom commented Jan 18, 2022

@w1th0utnam3 Thanks for the improvements you've done!

  • Pretty sure tkinter is the module to keep an eye on. Here's a working piece of code you can run right now:
from tkinter import *
from tkinter import filedialog

def openFile():
    filepath = filedialog.askopenfilename(initialdir="C:\\Users\\Cakow\\PycharmProjects\\Main",
                                          title="Open file okay?",
                                          filetypes= (("text files","*.txt"),
                                          ("all files","*.*")))
    file = open(filepath,'r')
    print(file.read())
    file.close()

window = Tk()
button = Button(text="Open",command=openFile)
button.pack()
window.mainloop()

Regarding availability of tkinter module, from official python docs: Link to segment in documentation

The tkinter package (“Tk interface”) is the standard Python interface to the Tcl/Tk GUI toolkit. Both Tk and tkinter are available on most Unix platforms, including macOS, as well as on Windows systems.

I think themeing implementation is also possible with tkinter. Default filedialog from tkinter doesn't look all that bad though.

  • python bindings for Dart

This project. Although It introduces a dependency from outside of dart:core which might not be preferable.


I just have some bits and pieces of the puzzle here and there, but no knowledge of actually stitching everything into a working prototype. 😅

@w1th0utnam3
Copy link
Contributor Author

w1th0utnam3 commented Jan 18, 2022

Regarding availability of tkinter module, from official python docs: Link to segment in documentation

The tkinter package (“Tk interface”) is the standard Python interface to the Tcl/Tk GUI toolkit. Both Tk and tkinter are available on most Unix platforms, including macOS, as well as on Windows systems.

Ah ok, looks good! However, "are available" does not necessarily mean installed by default. On some (older?) distributions it seems to be required to install the python package manually using the distributions package manager or pip. Though I didn't find something general. I guess this should be investigated further.

@philenius
Copy link
Collaborator

@w1th0utnam3 I completed the refactoring using the Factory Method design pattern and pushed my changes to your fork. There is now an abstract class DialogHandler which acts as an interface and factory for KDialogHandler and QarmaAndZenityHandler (plus future potential dialog tools other than kdialog, qarma, and zenity).

Ok, maybe you could take this into account while refactoring and allow the different dialog providers to also do post-processing of the picked files? Though we have to be careful with the space separator as we cannot just split at every space. I guess we would have to split such that each file starts with a / . Otherwise I think it would make more sense to just ignore the --multiple flag in the KDialog provider.

  • Post-processing: thank you for this hint, I took it into consideration. The post-processing was previously done by resultStringToFilePaths(). I added resultStringToFilePaths() to the interface of DialogHandler so that it must be implemented by KDialogHandler and QarmaAndZenityHandler.
  • Space separator: thanks again, due to your comment I realized that the current implementation would fail if a file name contains a vertical pipe (qarma and zenity separate multiple files by |). I already fixed the implementation for qarma and zenity by splitting by |/. Although, there is no perfect solution to this problem. The same problem exists on macOS where multiple files are separated by , alias. Even if doesn't work perfectly, we must keep the feature to pick multiple files. Removing it would be a breaking change.

@w1th0utnam3 : please let me know if are you interested to implement the missing implementations in lib/src/linux/kdialog_handler.dart (see the TODOs / UnimplementedErrors)? I haven't implemented the methods fileTypeToFileFilter() and resultStringToFilePaths() on purpose to leave something for you 😉 Please note that this task would also include the implementation of unit tests equivalent to those in test/linux/qarma_and_zenity_handler_test.dart. I could do it easily if you don't have the time.

Regarding the start/initial directory: let's first merge this MR and after that we can work on the integration of #859 .

Good point, we should include a note about unit tests in our contribution guidelines! 👍

I'll get back to yours and @mnipritom 's comments later.

@w1th0utnam3
Copy link
Contributor Author

Even if doesn't work perfectly, we must keep the feature to pick multiple files. Removing it would be a breaking change.

Sorry, I meant more like ignoring the flag for the KDialog implementation, not removing the option in the library. But hopefully we can get it to work now.

please let me know if are you interested to implement the missing implementations in lib/src/linux/kdialog_handler.dart (see the TODOs / UnimplementedErrors)?

Sure, I'll take a look!

@w1th0utnam3
Copy link
Contributor Author

w1th0utnam3 commented Jan 20, 2022

Ok, I implemented the missing functions and tests.

While writing the tests I noticed that the separation of paths using spaces by KDialog is of course much worse than it is with pipes. This prevents you from correctly parsing paths that have spaces at the end of a directory name in any part of the path. Any idea how we should handle this? I think in the multiple file selection case we are lost with spaces as separators. For the single file selection mode however we could just skip this splitting?

The design of the KDialog CLI is really questionable...

With the KDialog option "--separate-output" each selected file is
printed in a separate line. This simplifies the post-processing to
splitting by "\n".
@philenius
Copy link
Collaborator

@w1th0utnam3 great job, thank you 🚀

While writing the tests I noticed that the separation of paths using spaces by KDialog is of course much worse than it is with pipes. This prevents you from correctly parsing paths that have spaces at the end of a directory name in any part of the path. Any idea how we should handle this? I think in the multiple file selection case we are lost with spaces as separators.

I agree with you. The approach of zenity/qarma to use pipes as separator is a little bit better because it's a rather uncommon character. But choosing blanks/spaces as separator creates nightmares. Luckily, KDialog offers the option --separate-output which results in each selected file being printed in a separate line:

$ kdialog --help
...
  --separate-output         Return list items on separate lines (for checklist option and file open with --multiple)
...

This way, we can simply split by \n. I changed your implementation so that --separate-output is added as an argument when multiple files are selected and changed the separator in resultStringToFilePaths() to the new line character.


For the single file selection mode however we could just skip this splitting?

No need to distinguish the two cases (single file vs multiple files). Splitting by \n works in both cases. I tested it:

Peek 2022-01-20 23-24


The design of the KDialog CLI is really questionable...

Same for zenity and qarma. The problem is that we chose this script based approach where we execute shell commands. For our Windows implementation, we call the API of a Windows DLL via Dart ffi package. This approach is much more safe and stable (so far). But the script based approach for macOS and Linux has a lot of limitations, e.g.:

  • KDialog, qarma, or zenity are not available on all Linux distributions
  • There are always limitations when it comes to the post-processing of the std output
  • UX limitations like keeping the file picker dialog in foreground like a modal

A Flutter contributor/Google employee warned us about this early on: #630 (comment). Unfortunately, we lack the skills to do it the professional way like Google did (C++ code which calls APIs of the operating system, see https://github.com/google/flutter-desktop-embedding/tree/master/plugins/file_selector).

Copy link
Collaborator

@philenius philenius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: lib/src/file_picker_linux.dart has been moved/renamed to lib/src/linux/file_picker_linux.dart. Git just doesn't recognize it because this file has been renamed and some of its code has been extracted to other classes.

@philenius
Copy link
Collaborator

philenius commented Jan 20, 2022

@miguelpruivo we are ready to merge! Are you okay with these changes? We created a sub-directory lib/src/linux/ which contains the linux implementation and refactored the code using the design pattern Factory Method. File type filters work now on KDE Plasma and multiple files can be selected, too. We also added the missing unit tests and a first version of contribution guidelines in CONTRIBUTING.md.

@mnipritom
Copy link
Contributor

Really appreciate you guys iterating upon the crude initial implementation I made and coming up with something way more comprehensive. This has been a learning experience for me too and I grateful to both of you for it 👍

@philenius
Copy link
Collaborator

@mnipritom thank you for the kind words :) It wouldn't have been possible without your initial contribution. Thanks to you I learned about the existence of KDialog on KDE. It made me remember issue #839 from September 2021 by prateekmedia. I saw that you have contributed to his Flutter app AppImagePool, too. Back then I had to tell prateekmedia that we only support zenity and qarma. This is different now thanks to your contribution.


Hi. This is exactly the type of feedback I was looking for in here #931 and thanks for providing it @philenius !

I'm glad to hear so. You're welcome and sorry for the delayed and indirect feedback. I sometimes take a few days to respond and in the case of your pull request I was definitely too slow. Thanks again!


I did not think it would be necessary to ensure strict measurements within this package over what type of files get picked, other than what is already implemented.

That's alright. Even with the changes introduced in this pull request, the user can still remove the file filter and select any kind of file. Developers have to double check the selected files either way:

Peek 2022-01-21 21-43


I have found a similar bug listing here, which is over 7 years old and I doubt is ever going to get addressed. Since majority of KDE community effort is going towards porting majority of the codebase over to the newer version of Qt framework.

Same goes of zenity and GTK4 porting efforts. Pretty sure qarma is a abandoned as well, not even LXQt distros are shipping it.

Thank you for summarizing your research regarding the file filtering and KDialog in general. I'm disappointed by zenity/qarma/kdialog, too. The development of many Linux/Ubuntu tools seems to have stagnated for many years.


That's just me being a noob, un-experienced, not going over project structure and general lack of familiarity with industry practices regarding software development. Apologies!

No problem at all and no need to apologize 😉 You did a great job!


That all being said, with all due respect, I think documentation could use some updates reflecting the expectations of maintainers, in the form of Contributor's guide as to how one should approach contribution to the project, as it is one of the most popular 3rd party packages on pub.dev and is set to bring a lot more indie adopters like me as Ubuntu will be developing most of it's utilities using Flutter going forward.

Thank you for your valuable feedback. I pushed a first version of our contribution guidelines in CONTRIBUTING.md. I'm looking forward to the future of Flutter apps on Ubuntu!


Also thank you for your detailed elaboration on an alternative implementation using Python. Regarding that, I completely agree with @w1th0utnam3. I really don't want to add Python code or dependencies on Python packages into our existing code base which already uses Dart + Java + Objective-C. In my opinion, kdialog, qarma, and zenity seem to be the better (but imperfect) alternative because they are more likely to be installed than the combination of Python and pip and tkinter/other packages.

@philenius philenius closed this Jan 21, 2022
@philenius philenius reopened this Jan 21, 2022
@w1th0utnam3
Copy link
Contributor Author

I agree with you. The approach of zenity/qarma to use pipes as separator is a little bit better because it's a rather uncommon character. But choosing blanks/spaces as separator creates nightmares. Luckily, KDialog offers the option --separate-output which results in each selected file being printed in a separate line:

Ah I missed this while looking to through the commandline arguments. Technically a filename might actually contain \ns but at least it's better than spaces. Of course you would have similar problems with the |. Seems like the only completely safe character is the / so they (KDialog devs etc.) would have to remove excess / entered by the user and then use / as a separator so you can split by // (or even at least than 3 / if you wanted to support URLs... but not sure if multiple consecutive slashes in a URL might actually be important for routing...).

But this is why I suggested skipping the splitting for the single file case. To at least have a correct implementation in the common case of selecting only a single file?

Should this be documented as platform specific behavior? Otherwise I could imagine someone filing in issue when they try to find out why their "special" files cannot be opened.

@philenius philenius merged commit 9c58307 into miguelpruivo:master Jan 26, 2022
@philenius
Copy link
Collaborator

@w1th0utnam3 the zenity/qarma implementation splits by |/ (pipe followed by slash) which is a safe approach because the forward slash cannot be part of a file name. I agree with you regarding the KDialog implementation. One would expect that the current KDialog implementation cannot handle file paths that contain \n. But miraculously it works... Just try it out using our example app 😉

  1. In single file mode, KDialog does not allow you to select a file that contains \n in its name (bug?). However, KDialog does not show an error if the file path (without file name) contains \n:
    image

  2. I debugged this and still I don't understand. There's gotta be some kind of escaping but I can't find it. See for yourself:
    Peek 2022-01-26 22-05

Should this be documented as platform specific behavior?

Right now, I don't think that we need to document anything because file_picker always correctly returns the picked file(s) (see screen recording). If you still find a bug, you're welcome to open a new issue / pull request or document platform specific restrictions/behavior in our wiki: https://github.com/miguelpruivo/flutter_file_picker/wiki/api#methods

Otherwise I could imagine someone filing in issue when they try to find out why their "special" files cannot be opened.

We already had such an issue on macOS (#890) where a user tried to pick a file with , (comma followed by a space) in its name. I "improved" the implementation so that it splits multiple files by ", alias ". But one day, there will be a user who names a file something, alias .txt...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
desktop The issue applies to Windows, Linux or MacOS implementations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants