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
Icons #399
Icons #399
Conversation
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.
This looks really good. I particularly like the "fallback to png then to bmp" approach you've added for Windows icon lookup; it would be good to have that behavior replicated on Cocoa as well.
I'm of two minds whether "return tiberius" or "raise error" is the best approach if an icon file can't be found. An application crash is never a great option; but it would be nice to get some form of visible warning if a resource is missing. Maybe we could print a message to the console log about the resource that couldn't be loaded, and display Tiberius as a fallback?
I'm pretty sure GTK is the only other backend that supports Icons at this point; if you could port this filename lookup strategy to that backend, that would be great. If you don't have access to Cocoa or GTK for testing, I can help with that testing once the code is in place.
Then I will add a console message similar to 'Not implemented' we get for the features that are not yet implemented in the backend; and fall back on TIBERIUS. I have just realized that icons are used both for the app icon and as menu item icons. These two cases seem to require different approach in winforms: app icon is .Icon property, and menu icons are an .Image property. I will try to address that. I'm not sure if it is the same in other backends. |
Large icons would make the app look like it does on mac and iOS, but we're real answer is "whatever would make the app seem most 'Windows native'". I don't have enough experience with Windows to really judge. The current appearance looks odd to me, but I'm also not a Windows user, and from what I remember of the last time I used MS Word, it looks vaguely familiar. So - if this is the most "windows-y" appearance, then that looks great! |
I would say that this definitely looks more windows-y than the screenshot of mac version of the same tutorial. |
I'm guessing the "modern" appearance you've pointed at is a UWP app, rather than Winforms? Longer term, if there's a way to write apps using that API (or with that appearance), that would be great - ideally, Toga apps shouldn't look "old". However, we're on Winforms for the moment; and I can see that the older app has the labels next to the icons... most of the time - there are some icons in that example that don't. Is there any particular reasoning behind when an icon will have a label? Is there a Windows User Interface Style Guide that we can reference here? Apple, for example, has the HIG; here's the section on toolbars. Assuming there is an underlying reason for having the label (e.g., "important" commands get a label), this could be integrated into the Command API. The Toga API tries to capture the underlying idea in UI designs; so we could add a way to register a command as being "important". The "important" flag would be ignored on most platforms, but on Windows, that would be interpreted as "include label" on the toolbar. |
(BTW: I'm only suggesting this extra handling if we can find a good interpretation for why text is/isn't included. If there isn't one, and including the text always makes sense, then we can just go with that) |
Actually, the "more modern" appearance is from a WPF app, and the older one looks like a Winforms application. While UWP is the newest system for Windows, I think the best choice for toga would be WPF because
Thank you for the link on Mac Interface Guidelines! There is actually similar document for Windows, section on toolbars, and this is the more modern UWP version of it. |
So... it depends :-) From the Mac perspective, every app has a menubar - but it appears at the top of the screen. A window might also have a toolbar, which would run across the top of the window and have icons. From the Windows perspective, only the main window would have a menubar; it might also have a toolbar. There is probably also a style of "Simple app" that doesn't have a menubar. From the Mac perspective, it will still need to have a menu, but it can be very minimal. Tutorial 2 defines a toolbar on the main window; however, all the commands that are registered automatically appear in the menu for the app. In that example, commands 1, 2, 3 and 4 should appear on the window's toolbar; but there's also a command 0 defined that should appear in the apps menu bar. Even though command 2 isn't explicitly added to the application, it should appear in the menubar - the menubar is the comprehensive list of all commands that can be accessed anywhere in the app. The menu bar will also contain all the basic items you'd expect an app to have - Quit, About, Help - and they should appear in the "right place" for the platform (e.g., Quit is the last item in the File menu on Windows, but it appears in the Application menu on macOS) Does that make sense? |
So, the app in the screenshot on the main page actually also has a menu, but it is hidden by default on Mac? I think it would be nice if we had a screenshot with menu as well. Here are some considerations for Windows implementation, based on the Windows guide:
All quotes are from Windows desktop apps design guide. |
The macOS screenshot shows the toolbar, but not the menu bar. The menu bar is across the top of the screen on a macOS, and doesn't form part of the window itself. It's a little inconvenience to capture in a screenshot - but here's what it looks like: Every command added to the toolbar is automatically added to the menu (on the idea that the menu is the way to get at all functionality, the toolbar is there to provide shortcuts). They can also be manually added to the app (and thus the menu bar); or only added to the app (as in the case of I understand what you're suggesting about having an "icon with text" command type; however, that runs slightly contrary to the approach I've been trying to take with Toga APIs. I've been trying to capture "high level" concepts, rather than specific manifestations of features. So - rather than exposing a specific feature that Windows provides (having text and an icon), the Toga approach is identify why an icon would need text and an icon, and use that as a trigger to use that icon type. This is because the "text and icon" style doesn't exist on any other platform - it's a design element that only exists in the Windows style guide. If an API allows you to specify "use text + icon style", but you won't actually get a text + icon, that could be confusing; or would lead to calls for us to manufacture a widget that does have that style on macOS, even though it would be completely alien on that platform. So - rather than explicitly saying "use text and an icon", the user would instruct the Toga API that the command was "important" (or whatever identifier makes sense); we can then describe how "important" commands are displayed on different platforms (no change on macOS; text + icon style on Window). Similarly, standard menu entries like File (or the application menu on macOS), and Help automatically get added to the app, and are populated with basic entries. Menus like Edit, View and Tools will only get added if there are editing, viewing, or tool access commands. Does that make sense? |
Thank you for the screenshots, they helped me understand what menu really for this app.
|
Ah! In which case, we might not need to expose any additional API. Toga ships with a bunch of built-in commands for common application functions (like Quit). The original idea is that those commands are ones that have known functionality, and will appear in known locations in menus; but if we expand that a little to include "known icons", then it also solves the problem with have with Windows toolbar icons: built-in commands are displayed as just icons; user-supplied commands are provided with a text label as well. Internally, that might mean builtin commands need a separate subclass of Command, or an internal (not exposed to the user) flag to indicate their "known" status, but that should be fairly straightforward to implement and manage. |
@obulat What's the current status of this PR? Is it ready for final review and merge, or did you have more pieces that you were hoping to add before then? |
I've started with Icons on the toolbars, went on to creating toolbars with icons, and then continued on working on menus because they come from some of the same commands that the toolbars have. |
No problem - I just wanted to check if this was waiting on me, or if you still had work you wanted to do. |
I added menu creation for winforms, including the standard menu items. Can you have a look? |
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.
This looks pretty good to me - I've flagged one small simplification, but otherwise, I can't find much to fault.
One question though - how are you performing your merges with master? I can see you've merged this with the most recent changes, but your PR shouldn't include all the content from the commits you've merged. The combined history makes it a bit difficult to work out what is new content from this PR, and what is something that has already been committed to master.
src/winforms/toga_winforms/window.py
Outdated
def handler(sender, event): | ||
return action(None) | ||
|
||
return handler |
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.
This looks like it's repeating code in create_toolbar
- can we pull this out as a general utility method that both toolbars and menus can use?
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.
Is this the simplification you mentioned in the comment? I really have a problem with git... What should I have done to merge all the changes to this branch in my fork?
Is winforms/libs a good place to put this kind of code (add_handler function)?
…mand icons for winforms
…mand icons for winforms
Signed-off-by: obulat <obulat@gmail.com>
Signed-off-by: obulat <obulat@gmail.com>
…mand icons for winforms
…mand icons for winforms
Signed-off-by: obulat <obulat@gmail.com>
Signed-off-by: obulat <obulat@gmail.com>
Signed-off-by: obulat <obulat@gmail.com>
…ntation Signed-off-by: obulat <obulat@gmail.com>
Signed-off-by: obulat <obulat@gmail.com>
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.
From a code perspective, this is looking really good. I noticed two small (and slightly related) things in my testing:
-
Some of the menu items should be disabled, but aren't - there's no app-defined handling for "Preferences" or "About", for example, so they should be disabled.
-
The buttons on the scroll window, and some of the menu items crash the app if you press them. I'm not sure what is happening with the buttons, but the problem menu items seem to be the ones that should be disabled (possibly they're pointing at actions that haven't been defined?)
I didn't notice that some menu items were disabled. Where are they disabled? I couldn't find anything in cocoa... Also, is it better to disable them or to delete completely, if the app creator doesn't provide any implementation? As for the buttons, there are two reasons. The first one is as you wrote, because there are no handlers associated with some commands ('About the app' etc). And the second one is due to the fact that async is not implemented in Winforms. That's why the buttons in the scrollview crash the app. |
Signed-off-by: obulat <obulat@gmail.com>
Ok, I found it and fixed it. Now the menu items are disabled if they have no actions. |
Awesome! And the explanation for asyncio support makes sense too; The windows error message wasn't especially helpful in identifying the source of the problem. I was about to push the merge button when I noticed one last problem. It's a variant on something we've seen before - the top of the scroll window looks like it's being overlapped by the menu bar (or toolbar) and the "up" arrow control on the scroll is missing. I'm guessing this is because the vertical shift is enough for the toolbar, but doesn't account for the menu bar as well. |
Signed-off-by: obulat <obulat@gmail.com>
Good catch. I added menu height to the vertical shift calculations, as well as content refresh after creating menus. |
Moved the icons file resolution to the backend for cocoa and winforms.
Now, backends can get their proprietary file format ('.icns' for cocoa and '.ico') or create icons from '.bmp' or '.png' files. (Although I didn't find a bmp icon file, so I only tested ico and png on winforms.) I also adjusted tests because now core/icon doesn't automatically add '.icns' to the icon filename.
If this is acceptable, we will need to test and update other backends, too.
There is an issue about lack of icons in windows in briefcase.
PR Checklist: