Closed Bug 149841 Opened 19 years ago Closed 19 years ago
It should NOT be possible to open cert manager twice at the same time
Go to Edit/Prefs/Security/Certificates. Click "manage certificates". The certificate manager opens. Move away the new window, but do not close it. Find the open prefs window. Click again on "manage certificates". Actual behaviour: Another certificate manager window will open. If you modify one window by deleting or importing certs, the other open cert cert window will not disappear. (In addition, it will most like crash, when you delete a cert in one window, and try to work with that cert in the other open window). Expected behaviour: When you click the "manager certificates" button again, there should not open another window. Instead, the already open window should come to the front.
Forgot the word NOT in the summary, adding.
Summary: It should be possible to open cert manager twice at the same time → It should NOT be possible to open cert manager twice at the same time
Javi, can you please review?
Comment on attachment 93001 [details] [diff] [review] Patch v1 does calling focus really make it so that you can't open a new one? Shouldn't we look at making the window modal instead?
Calling focus is just a secondary thing to make it work. I do it to bring an already open window to front. The real trick is the changed window.open() call. By giving the window a name, subsequent calls to window.open() will re-use the previous window, and not open a second one.
Comment on attachment 93001 [details] [diff] [review] Patch v1 marking patch as has-review
Attachment #93001 - Flags: review+
Alec, can you please review?
Comment on attachment 93001 [details] [diff] [review] Patch v1 hmm.. can't you just make this modal? I'm not sure how focusing the window will have the desired effect.
Alec, I believe it was a design decision to make the certificate non modal. I was not involved in that decision, it happened before I worked on PSM. We would need to find the people involved in the original decision, but I believe making it modal is not wanted. My patch is just a small effort to make the existing behaviour more robust. Alec, if you know of a function "bringToFront()", please let me know. I could not find any, but it would probably more sense. At least on Linux, this function brought the window to front when hidden behind other windows. In any way, the major motivation is to avoid that certificate manager gets opened multiple times.
all I mean is that the focus stuff in the patch doesn't do anything - newly opened windows already have focus, and this doesn't prevent me from opening a second window. Ah, now I see why it appears that you avoided opening a new window! It has nothing to do with the w.focus(). The reason is that the patch adds the window target in openCertManager() - so that when you call window.open, the window will re-open in the same chrome window..a good test would be to change something in the window without saving anything, and then try to open the window again - what you should find is that any changes you made were reset. I think you're going to have to cache the result of window.open or something, and clear it when the window closes. You could also try the window cycling code that the taskbar uses (the mozilla taskbar, not the windows taskbar) to find the most recently used window of a given type.
*** Bug 162034 has been marked as a duplicate of this bug. ***
I agree. Probably the most similar code I can re-use will be the logic for the preferences window, since it is modeless, too.
this patch finds the most resent opened window and focuses on it.
Comment on attachment 115351 [details] [diff] [review] patch Great patch, thanks! I already tried it, it works for me. r=kaie
Comment on attachment 93001 [details] [diff] [review] Patch v1 marking my earlier patch as obsolete
Attachment #93001 - Attachment is obsolete: true
Attachment #115351 - Flags: review?(kaie) → review+
only modifies the comment a little since I directly copied the code from the preference window code.
Attachment #115351 - Attachment is obsolete: true
Comment on attachment 115353 [details] [diff] [review] patch take over r=kaie and seeking sr=?
Comment on attachment 115353 [details] [diff] [review] patch sr=alecf
Attachment #115353 - Flags: superreview?(alecf) → superreview+
checked in. Thanks!
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.