Summary: | Impossible to close web inspector when docked to webview | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Matt Lilek <dev+webkit> | ||||||||||||||
Component: | Web Inspector (Deprecated) | Assignee: | Timothy Hatcher <timothy> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | aroben, kmccullough, ono, rik, timothy | ||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||
Version: | 523.x (Safari 3) | ||||||||||||||||
Hardware: | All | ||||||||||||||||
OS: | All | ||||||||||||||||
Bug Depends on: | |||||||||||||||||
Bug Blocks: | 19477 | ||||||||||||||||
Attachments: |
|
Description
Matt Lilek
2007-06-21 00:04:49 PDT
Created attachment 18795 [details]
patch
Add a close button to the toolbar when the inspector is docked. After the inspector is closed this way, it reopens docked. I think that makes the most sense but I'd be more than happy to listen to reasons for why it should be another way.
The images used for the close button are from Drosera, renamed and converted to pngs to match the rest of the inspector's images.
Build fix stubs of the new client method for non-mac/win platforms are not included in this patch, but I have them locally.
Comment on attachment 18795 [details]
patch
+ { "attachedClose", attachedClose, kJSPropertyAttributeNone },
Maybe "hide" would be a better name for this method?
I think the Windows code could be better factored to share more code between closeWindow and closeAttachedWindow. It also seems strange to have to call ShowWindow(SW_HIDE) in closeAttachedWindow() -- won't th window already be hidden?
Is there any flashing when m_attachOnShow is true? I'd expect to see the detached window appear, then disappear given the way the code is written.
Any chance we could see some screenshots of the new close button in action?
(In reply to comment #3) > (From update of attachment 18795 [details] [edit]) > + { "attachedClose", attachedClose, kJSPropertyAttributeNone }, > > Maybe "hide" would be a better name for this method? In fact, maybe the button should even be a sort of "minimize" button, since presumably the effect is that the Inspector animates to 0 height and disappears. Created attachment 18836 [details] revised patch This is a revised patch based on Adam's comments, but I'm not putting it up for official review. It's mainly to see whether my Windows code is horribly wrong or not. (In reply to comment #3) > (From update of attachment 18795 [details] [edit]) > + { "attachedClose", attachedClose, kJSPropertyAttributeNone }, > > Maybe "hide" would be a better name for this method? > > I think the Windows code could be better factored to share more code between > closeWindow and closeAttachedWindow. It also seems strange to have to call > ShowWindow(SW_HIDE) in closeAttachedWindow() -- won't th window already be > hidden? > Thats what you'd think, but I'm not 100% sure - I don't really know enough about Windows' windowing system. If my revised code is still wrong, I'd love pointers to figuring out what I'm screwing up. > Is there any flashing when m_attachOnShow is true? I'd expect to see the > detached window appear, then disappear given the way the code is written. > Yes, I overlooked that in my initial patch. It's taken care of in this revision. > Any chance we could see some screenshots of the new close button in action? > I'll upload a screenshot of it in a moment. If the general style is okay, I'll photoshop it to be a - instead of an X. Created attachment 18837 [details]
screenshot
screenshot - not exactly a whole lot to show
Comment on attachment 18836 [details]
revised patch
I don't think you need the m_shouldAttachOnShow member. m_attached should already have the value you want.
I think to make the code clearer, we should separate these two concepts:
1. Moving the Inspector's WebView into/out of the inspected WebView's window
2. Moving the Inspector's WebView into/out of the Inspector's own window
So we could have private methods like:
insertIntoInspectedWindow()
removeFromInspectedWindow()
insertIntoOwnWindow()
removeFromOwnWindow()
If we do that, then I think we'll have these nice simple (pseudocode) implementations of the public methods:
WebInspectorClient::attachWindow() {
removeFromOwnWindow();
ShowWindow(m_hwnd, SW_HIDE);
insertIntoInspectedWindow();
}
WebInspectorClient::detachWindow() {
removeFromInspectedWindow();
insertIntoOwnWindow();
ShowWindow(m_hwnd, SW_SHOW);
}
WebInspectorClient::hideWindow() {
removeFromInspectedWindow();
}
Depending on how the methods are coded you might need to keep track of some state to know whether you're actually in a window at all (since you won't be after hideWindow() is called). But I think this should make things much cleaner.
Any news to this bug? With latest WebKit it is still impossible to close docked inspector. Also it isn't possible to resize docked inspector. Would be nice if I dock the inspector Opt+Cmd+I hides inspector, Opt+Cmd+I shows it again docked. Created attachment 22768 [details]
Proposed patch
Created attachment 22769 [details]
Picture
Created attachment 22770 [details]
Proposed patch (part 2)
Thanks for the patch Timothy, looks great. However does it close the inspector on Opt+Cmd+I as well? (So on docked mode Opt+Cmd+I is a key to toggle the inspector, rather just to open it). No, this just adds a close button. Please file a Safari bug for that, since it is a Safari key command. |