-
-
Notifications
You must be signed in to change notification settings - Fork 671
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
Windows - implement modern File/Folder dialogs with comtypes
#2786
base: main
Are you sure you want to change the base?
Conversation
comtypes
comtypes
Thanks for the suggestion and contribution! I'd definitely prefer to use the newer form of dialogs if at all possible. The Winforms look and feel definitely appears dated (and, as you've noted, are also feature limited). There has been some discussion about adding a WinUI3 backend to Toga (see #2574); if The verboseness of the With my own background building Rubicon for ObjC, I can see broadly what it's doing; my immediate code review suggestion would be to do some refactoring, and separate the parts that are literal wrappers of COM objects into the As for how to run tests of the dialogs - that's performed as part of the testbed. The contribution guide contains details on how to run the testbed suite; however, there will almost certainly be a need for changes to the winforms test probe to support testing the new dialogs. CI is also currently failing because of pre-commit checks; the contribution guide also contains details on how you can set up your environment to perform pre-commit checks etc. Some other "big picture" thoughts:
|
also organize constants/prototypes into libs
@freakboy3742 Thank you for making a newbie feel like this code isn't so far out of place in this library I still don't understand fully. Since you mentioned winforms is deprecated, in the dialog code at least I've replaced all the messageboxes and file dialogs with ctypes/comtypes implementations. The most difficult was the StackTraceDialog. I also added other various windows libs that may be useful to Toga, including how to render the windows context menu (explained in more detail here) Implementing pure python solutions where possible I believe will make it easier for developers to understand Toga's internals, since they can literally step through the pip package code directly in their debugger for the dialogs, instead of wondering what kind of sorcery clr/winforms is doing under the hood. I am extremely open to further improvements, please tell me any existing concerns and the best way to continue implementing into the existing Toga library. |
I have since improved my current custom test script since I'm still unsure how to test the dialogs the proper way. I realize this is not relevant to the pr but I am struggling to solve this issue, preventing me from fully integrating my dev environment:
I've since tried to reinstall the .venv double checking the instructions on the toga contribution docs but I haven't had any noticeable success. |
As I understand it, it's not deprecated - it's just not "best current practice".
I'm not sure I agree on this point. Your most recent updates add a lot of code - but almost none of that code are "intuitively obvious" Python code. They're very mechanical reproductions of very specific COM APIs. Compare and constrast the basic operation of the Python.NET Dialog wrappers to what you've presented in this PR. The old code says:
The documentation for MessageBox can be discovered by a Google search for "Winforms MessageBox", and maps to Python in obvious and consistent ways. By contrast, your COM equivalent requires hundreds of lines of boilerplate to get started. Yes, it exposes features that aren't available; but it's a big stretch to claim that it's obviously better just because you can step through that COM binding in a debugger. This is why I mentioned the possibility of a Rubicon-like reflection layer for COM. That would help us automate (or semi automate) the production of these API wrappers.
The first changes required are the changenote and linting checks that CI is currently validating. The Toga contribution guide has details on how to setup your development environment (noting the Python 3.8 issue you've reported separately). The second major concern right now is the provenance of the code you're contributing. Can you confirm that you're the original author of all this code, and that you're in a position to contribute it to an Open Source project? From a surface inspection, it looks like a some of this code may have been copied from https://github.com/NickHugi/PyKotor - which is a problem if you're not the original author of that code, as we need to ensure that we credit the original author, under whatever license terms they offer. |
Ohhh ok your original post makes a bit more sense now. This indeed would keep the eyes of other devs away from the 'hundreds of lines of code required to get started with com interfaces.' That level of knowledge is beyond me. I have been using I'm not sure what the best solution is. This PR could be as simple as defining a replacement for SelectFolderDialog/OpenFileDialog, which would be significantly less code (first commit), or it could be implemented as a replacement for any windows dialogs if
Thanks for the reminder, I'll take care of this this weekend and get the CI validating.
I appreciate your concern regarding the provenance of the code. I can confirm that I am the original author and maintainer of the code in question, which was initially developed for the PyKotor project. I am the current maintainer of the PyKotor repo, where the code originated. For the purpose of contributing to Toga, I am re-licensing this specific code under the MIT license, which I understand is compatible with Toga’s licensing. This ensures that the code can be freely incorporated into the Toga project." I'm just a guy tired of seeing SHBrowseForFolder used in 2024. |
I'd like to further clarify that I think, despite the excessive nature of the interface definitions themselves, this is fairly standard to my knowledge. if I may point attention towards how this is written in C++: https://github.com/th3w1zard1/CTestIFileDialog/blob/main/IFileDialog.h attempting a 1:1 port of the same code to Python and it's not too different. In C#, it would look similar to this stackoverflow answer. Your example using:
is just highly unfamiliar to me. I have not worked with winforms before. Perhaps it is the better way to implement those message boxes. In that case I should revert all commits here except the first? Some thoughts that cross my mind:
I am not at all familiar with Toga, its goals, best practices, and other PRs. I'd rather someone with that experience tell me what this pr can and should address, so that it is properly addressing the things it needs to address without becoming overzealous in its approach (last few commits) |
It's the steep learning curve that is my concern. We're aiming to make the codebase as approachable as possible, and "you need to be able to read Win32 API C++ header definitions"... isn't very approachable :-)
At least for this first pass, I'd be inclined to keep it minimal - recognising that the stock Winforms file selection dialogs aren't current best practice, and replacing just those, with as little wrapper code as is needed to make that work. That gives us an opportunity to evaluate the impact of a COM-based approach before we try a more substantial adoption.
If you're the original author, and the code is being explicitly contributed to Toga, then we're essentially asking that you authorise it to be released as part to the Toga codebase, under a BSD-3-clause license. Relicensing parts of the code under a second license is something that complicates the redistribution process, and is only required if we're using code from somewhere else, rather than having code given to us.
Ok - but that's C++ code in your repository. What's the upstream source of truth for this? If I, a non-Microsoft developer, wanted to discover the same information - where do I go?
I think you're missing my point. What I've presented here is Python code, not C# code. It is all the Python code you need to invoke a Winforms class. The official C# API documentation is easily searchable, using the keywords that are in this source code. Having read that there is a C# class called "System.Windows.Forms.MessageBox" that has a method named "Show", you essentially know everything you need to know to invoke that class from Python. And if you didn't know the class was called MessageBox, a Google search for "Winforms Info Dialog Box" would get you to this page pretty quickly. What is the equivalent path for the COM approach? You seem to be suggesting shobjidl_core docs as an entry point... I have no idea what "shobjidl" means, but it doesn't bear any resemblance to "Winforms", "WinUI", "UWP" or "COM". If I get to the IFileDialog page, it tells me, for example, there's a method named "SetFileNameLabel", described as: HRESULT SetFileNameLabel( Which... what does that mean? The MessageBox docs contain examples of usage. The shobjidl_core docs contain... words. I don't even know what I'm looking at. If you're a C++ Windows GUI programmer, maybe this information makes sense - but it doesn't make any sense to me. And I'm coming at this with experience building a GUI abstraction for Windows, macOS, iOS, Android, GTK, Textual and the Web, and experience in probably half a dozen other GUI frameworks. I shudder to think what the experience is like for someone without my experience.
I'm not sure I understand what you're asking. If a feature isn't implemented, it isn't implemented. There's no magic way to make it available if the platform doesn't provide it. Abstractions, by definition, leak. The best you can do is build the best abstraction you can. You can see that right now in the Winforms dialog implementations. There's a public interface that exposes a bunch of features; Winforms doesn't support some of those features, so the API adapts as well as it can. And yes, that means that some features aren't exposed by default. There will always be things you can do if you're writing directly against a platform API that you can't do if you're writing against an abstraction API. But - if you use an abstraction API, your code can run on multiple platforms. Winforms code will only ever run on Windows. It's a tradeoff.
It's relevant if spending a week building an abstraction interface once means you don't ever need to define another C++ wrapper. Take the macOS example. If you want to use NSAlert, you don't have to read the Objective C source, and then manually transcribe each of the APIs on NSAlert that you might want to use. You declare that you want to use NSAlert, and call any documented method on that class. The entire Python interface to NSAlert is:
Well, as the founder and principal maintainer of the project, that's what I'm aiming to do. Yes - you're exposing a dialog interface that is more modern, and has more features. That's a useful feature to add. If COM is the only way we can get to that new Dialog interface, then that's something we may need to accept. What I'm wary of is that your contribution here isn't "here's the same interface you've been using historically, extended to implement a new feature". It's a radically different approach to how to build the interface at all. And, as such, that comes with tradeoffs, and documentation requirements, and more. It requires consideration not just of "does this work", but "what does maintaining this for the next 10 years look like?", "is this the best approach to achieve the ends we're looking for?" - and in the most mercenary question - "could I maintain this if the contributor disappears after this PR is merged?". Right now, the answer to these questions is, at best, "maybe". We also need to answer the question of whether it's worth throwing more effort down the Winforms API hole if the rest of Windows is moving to other APIs. Should we be focussing on WinUI3? Should we build a COM-based backend? This is an area where I have almost no expertise - I don't use Windows regularly, and I haven't actively developed on Windows as a primary platform for over 30 years. Building a new backend is a big job; so I don't want to go down that path unless I believe that it's a path worth dedicating resources so. |
Clearly separate identifiers, interfaces, and constants for PR state
I would like to apologize for any confusion or misunderstandings that have come up. I will do my best to address these points. This is my first time contributing, not only to Toga but a project of this size and magnitude. I don't know what's expected of me in this situation. I definitely chose the 'create a pr and figure it all out later' approach and it clearly is not working at all for me. I have fully read the contributors guide, and have setup my development environment. Thank you for your patience. Anything except the Common Item dialogs are beyond me, therefore I have removed all code related to them from this pr. I have left a few interfaces and defined them in more_interfaces.py. This file contains the full potential of these dialogs, and allows implementation at a later date. The new additions could still be significantly shortened but not without losing readability (e.g. the static type hints).
Yes many of these thoughts ran through my head as well, before submitting this PR. I was actually hoping the community would be able to answer these questions after I submitted the pr, either indirectly or over time. I've briefly looked at what Qt and Electron are doing and that rabbithole brought me to the concept of regardless if this pr must collect dust in hopes of a better solution may come along that is acceptable. I do not mind updating this PR every week or so to keep conflicts out of the gutter. At this time I believe I've done everything I can within the scope of this PR: replacing SHBrowseForFolder. let me know if there's any further improvements/fixes that need to be addressed within the scope of this pr. I definitely do not want to be difficult I just want to learn and contribute. I look forward to seeing what the future holds for these dialogs. Thank you for pickling my brain with much, much information I had not considered. |
No problem at all - thanks for the considered responses to my feedback. FWIW - if I wasn't interested in this direction at all, I'd say so. There's definitely something worth pursuing here; but it's not a simple change, and we need to work out the best way to implement that change.
The PR is definitely a good start - if nothing else, it proves you're not just "kicking the tires" - you've made a concrete suggestion, and providing a solid concrete first-pass implementation of that suggestion. You're not just handwaving about something we "should" do, and telling us to go make it happen for you. Even if it's not "fully correct" on the first pass, the effort is definitely appreciated, and I want to work out how to get this from the first implementation to a form we can work with long term.
That sounds like a good approach from a scope perspective. If the scope is deliberately minimal, the need for an automated COM wrapper is also lessened (at least for this pass); we can defer those discussions a bit until it's clear COM wrapping is something we're going to need to do a lot of.
Oh sure - and it's the answer to those questions that I want to resolve. As I said - if we didn't want this, I'd say "thanks but no thanks". But this is also an area where I'm not deeply familiar with the state of the art. Toga has a Windows backend because it's an important platform, not because I have any deep personal interest or experience with the platform - so I'm going to be asking some pointed (and sometimes open ended) questions trying to work out what the best approach will be.
From my perspective, having this PR live in eternal unmerged limbo is the worst possible outcome. There's either something here we can use, or we should close the door and stop wasting your time. FWIW, I don't think it's the latter - I honestly think there is something we can use here; we just need to work out how to make it maintainable by "not you" in the longer term. In terms of immediate work that needs to be done; the first thing that needs to be resolved is the testbed test. It's currently failing because you've introduced a dependency on comtypes, but the toga-winforms backend doesn't declare that depdendency. Once that issue is resolved, you'll likely find that the testbed tests themselves will either fail or lock up because the test probe doesn't know what to do with the new dialogs. Longer term, there's some code organisation stuff that still needs to be cleaned up, and we'll likely want to add some documentation as a guide to future code explorers on how to read/interpret the COM wrappers. |
Am I correct to assume that these PRs usually don't intend to tackle everything at once, but instead provide a starting point for where future implementations can continue off of? If so, how do you recommend I structure my current changes to entice that? Would that mostly consist of documentation? Thanks for the wholesome response, and the context of where you are coming from. I'm mostly a Windows developer that is interested in learning linux/mac by branching out to relevant api's and libraries.
OK no problem. I'm looking at these tests now and I'm not finding what I've expected to see. I was expecting to see a test configuration for either pytest or unittest, where the arguments and return values are correct. These seem to actually be testing the dialogs and ensuring they run and return the correct results. How is that possible without mocking or patching?? |
@@ -66,6 +66,7 @@ test_sources = [ | |||
] | |||
requires = [ | |||
"../winforms", | |||
"comtypes", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While this fixes the immediate problem, it's not the right fix. You're proposing adding code to toga-winforms
that uses comtypes
; that means that comtypes
is a dependency of toga-winforms
, not the testbed. Testbed should be picking up comtypes transitively because of the dependency on toga-winforms
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, you're absolutely correct.
Each PR is fully self-contained - so whatever feature it adds needs to be complete, including tests and documentation. However, that doesn't mean it needs to solve every problem all at once. It's fine to have a complete solution of part of the overall problem. Identifying the scope for the "smallest version of a new feature" is the key idea. In this case - I think the scope is fine - "Upgrading the file dialogs for winforms". There's maybe a world in which you only tackle the "file open" dialog or something like that; but honestly, the three file dialogs are close enough that the benefit from splitting the work into multiple parts like that would be minimal.
In which case, we definitely don't want to scare you off :-) We definitely need people who know Windows conventions through regular use, and are familiar with the underlying development ecosystem.
The contribution guide contains details on how the testbed works. The short version is that the testbed is a "working app" that has a test suite; the tests are backed by "probes" provided by each backend that are implemented on each platform. Each test will call the public APIs to make the GUI do something interesting; and evaluate a generic test of "this is what should have happened"; the probes then use platform-specific probe logic to confirm (to the extent possible) that the GUI actually did change in the expected way. |
I became tired of using the legacy SHBrowseForFolder inside Toga, so I decided to implement IFileOpenDialog/IFileSaveDIalog, part of (the 'new' windows vista common item dialogs) into Toga directly.
I attempted to keep the implementation as light-weight as I could, therefore besides the dialog itself there are no new features being supported. For a complete standalone implementation and examples of what IFileOpenDialog actually supports, that can be ran here (todo: events & IFileDIalogCustomize interface)
This attempts to keep backwards compatibility intact, and abstracts through the
future
object to make that happen.I could not figure out how to run specifically the file dialog tests. After running
tox -m test
it passed successfully but it didn't seem to run any of my dialogs. So I created another test script to ensure they workAdvantages of IFileOpenDialog/IFileSaveDialog Implementation
Modern API Usage:
These APIs offer better integration with the Windows shell, supporting modern features like Places Bar, default folders, and custom filters.
Extended Functionality:
Improved User Experience:
The modern dialogs are designed to align with current user expectations, offering features like inline folder creation, search integration, and better accessibility support.
The inclusion of a preview pane, commonly found in IFileOpenDialog, enhances the user experience by allowing users to preview files before opening them.
Future-Proofing:
As Windows continues to evolve, relying on modern APIs like IFileOpenDialog and IFileSaveDialog ensures that the application remains compatible with future versions of Windows and can take advantage of new features as they become available.
Potential Drawbacks
Increased Complexity: The comtypes-based implementation is more complex than the WinForms abstraction. This could increase the maintenance burden and make the codebase harder to understand for new contributors.
Testing COM interfaces in an automated way might be more challenging, although the modern APIs generally have robust documentation and community support.
Dependencies:
Introduces comtypes (
pip install comtypes
) as a dependency, this is a pure Python implementation of COM programming.Bulkiness: As can be seen, the IFileOpenDialog and IFileSaveDialog implementations are bulkier than the WinForms abstraction. This added bulk could be seen as a disadvantage if the additional features are not considered essential for the Toga framework.
PR Checklist: