Even more widget properties #14
Conversation
|
I am sorry for the delay in reviewing this. Given that this is a large PR, I will try to review it in small chunks, and add a comment or few here and there until it is fully reviewed. Thank you for your patience. Feel free to address my feedback gradually too even before I finish reviewing everything. |
| @@ -32,19 +32,43 @@ class ToplevelProxy < WidgetProxy | |||
| DEFAULT_HEIGHT = 0 | |||
|
|
|||
| attr_reader :tk | |||
| attr_accessor :escapable | |||
| attr_accessor :escapable, :modal, :centered | |||
There was a problem hiding this comment.
I just tested the modal behavior. It hides the previous window on every modal window opening. That is not standard behavior. Also, the Mac environment does support real modals as demonstrated in samples/hello/hello_toplevel.rb using mac_style 'utility'.
Given that the modal behavior is not standard, it should be called something else to avoid confusing users. How about calling it hiding_modal or something else (if you can't come up with the best name, call it anything, and I could rename it later)?
Also, the real modal behavior on the Mac as demonstrated in Hello, Toplevel, supports pressing ESC button to quickly close the modal and go back to the main window.
I think you should support that behavior with the hiding_modal too.
Last but not least, please add examples of using the new hiding_modal behavior under samples/hello/hello_toplevel.rb
Ideally, all new behavior is added to samples to demonstrate the new features as well (or at least, they should be mentioned in the README.md).
By the way, you might be able to implement real modal behavior for Windows and Linux too (without hiding yet by disabling access to main window) as per this link (I don't know if it applies as it comes from Tkinter in Python, but maybe it works in Ruby too and could be automated behind a modal option for Windows/Linux):
https://stackoverflow.com/questions/16803686/how-to-create-a-modal-dialog-in-tkinter
There was a problem hiding this comment.
Hello Andy!
I'm also sorry for being slow in my responses, the circumstances not always allow me to dedicate as much time for personal projects as I'd like to :(
Yes, you're correct, that's not a standard behavior. May be my choice of the method's name is wrong. How about call it hide_root? Because of that's basically what it does - hides the root window when we want to make the user interact with the current toplevel only.
Yes, I tried the approach which also described by the link you provided. And in fact it is still here :) https://github.com/AndyObtiva/glimmer-dsl-tk/pull/14/files#diff-79d73888fd83581749673cadbbcc881b503ad66f694728be37c41415d401210aR55
but I found the root window still exists and the user may switch to it occasionally and lose the toplevel we want them to stay in. I found that may be confusing for the user, that's why I came to this solution.
So, hide_root? :) Or hide_root_window for more readability? :)
There was a problem hiding this comment.
Sure, I'll add examples. Sorry, I should have thought about that myself. As soon as I have time :(
There was a problem hiding this comment.
Do not worry about having the time to respond. I appreciate the time you already put into the PR. And, I am intentionally dividing the PR review into small parts to make it possible for both of us to respond gradually in small replies without having to spend too much time responding to everything at the same time.
Anyways, given that we cannot implement true modality for windows/linux (as per the provided link), then you can keep the attribute name the same (modal) for now.
But, in the future, I will likely make it activate mac_style 'utility' on the Mac only to provide true modality on the Mac at least. I think that would be the best compromise. You do not have to make any changes for the modal attribute though. I'll merge your work as is for modal, and will make the change myself in the future.
Otherwise, do not worry about the examples if it's not a quick task. I could come up with the examples myself and it would be a good exercise for me to better get acquainted with your code changes.
lib/glimmer/tk/toplevel_proxy.rb
Outdated
| center_within_screen unless @x || @y | ||
| center_within_screen if centered? |
There was a problem hiding this comment.
Centering was done as a smart default in the past unless the x and y attributes were specified. I think it is important to maintain this smart default without requiring the user to specify centered true.
This default could be changed to "centered within root" (instead of "centered within screen") for modals.
But, you can support centered false if you want to prevent centering when the user neither wants the default and nor wants to specify the x and y manually.
There was a problem hiding this comment.
Aha, I see your point :) I reworked the behavior a little - what do you think now?
lib/glimmer/tk/toplevel_proxy.rb
Outdated
| destroy | ||
| end | ||
| end | ||
| end | ||
|
|
||
| if is_a?(Glimmer::Tk::ToplevelProxy) && modal? |
There was a problem hiding this comment.
I am not sure why you are checking if is_a?(Glimmer::Tk::ToplevelProxy). The code is inside an instance method of Glimmer::Tk::ToplevelProxy, so self will always have is_a?(Glimmer::Tk::ToplevelProxy) as true.
There was a problem hiding this comment.
Because of RootProxy inherits from ToplevelProxy. And we probably don't want support modals for root :)
| end | ||
| end | ||
|
|
||
| if is_a?(Glimmer::Tk::ToplevelProxy) && @tk.iconphoto.nil? && root_parent_proxy.iconphoto |
There was a problem hiding this comment.
I am not sure why you are checking if is_a?(Glimmer::Tk::ToplevelProxy). The code is inside an instance method of Glimmer::Tk::ToplevelProxy, so self will always have is_a?(Glimmer::Tk::ToplevelProxy) as true.
There was a problem hiding this comment.
Because of RootProxy inherits from ToplevelProxy. My intention was to make able toplevels to inherit icon from root. If it's a root, nothing to inherit from :)
There was a problem hiding this comment.
That's not the way inheritance/polymorphism works though. RootProxy extends ToplevelProxy, so both ToplevelProxy and RootProxy will return true if you check is_a?(Glimmer::Tk::ToplevelProxy) (unless you check if the object.class == Glimmer::Tk::ToplevelProxy, which is usually not a good practice except in very rare situations). Only a superclass of ToplevelProxy would return false for that check, but it wouldn't have that code in the first place since it is the superclass, so then checking that condition would be unnecessary for a superclass (not your case though since you mentioned the subclass RootProxy). Maybe, you wanted to check is_a?(Glimmer::Tk::RootProxy) as that would return true only in RootProxy and false in a standard ToplevelProxy. But, usually checking for a subclass in a superclass is a code smell, so there are better ways of going about it, like by following the Template Method Design Pattern and deferring some of the logic to a method that has an empty implementation in ToplevelProxy and a filled in implementation in RootProxy.
Here is a GIRB (Glimmer IRB) example demonstrating my point above:
In any case, I will accept this code for now, and will update it myself in the future.
Thank you again for your contributions.
…sest_window, visible, modal
…; rework closest_window, visible, modal
No description provided.