GNOME Bugzilla – Bug 627259
Multiple "Could not display {filename}" alerts can open for the same file
Last modified: 2016-04-14 09:36:54 UTC
this report has been filed here: https://bugs.edge.launchpad.net/ubuntu/+source/nautilus/+bug/619780 "1. Download a file that Ubuntu does not know how to open, e.g. a Microsoft Cabinet archive file (.cab). 2. Double-click on it. 3. Double-click on it again. What happens: 2. An error alert opens, 'Could not display "/home/mpt/something.CAB". There is no application installed for Microsoft Cabinet archive files' 3. Another alert opens with exactly the same text. What should happen: 3. The existing error alert for that file is focused. http://www.youtube.com/watch?v=-N9xqN8Cf5Q"
Created attachment 325141 [details] [review] Make dialogs modal What if the dialogs were made modal?
Review of attachment 325141 [details] [review]: thanks for the patch! There are few issues/questions: ::: src/nautilus-mime-actions.c @@ -1109,3 @@ - g_signal_connect (dialog, - "response", unrelate change @@ -1115,2 @@ g_object_unref (location); - nautilus_file_unref (file); unrelate change @@ +1132,3 @@ + error_message); + gtk_message_dialog_format_secondary_text (GTK_MESSAGE_DIALOG (dialog), + _("The file is of an unknown type")); You lost the primary text? @@ +1143,3 @@ + error_message); + gtk_message_dialog_format_secondary_text (GTK_MESSAGE_DIALOG (dialog), + text); same here
Regarding the first two, there was some trailing whitespace I removed. The latter two, I thought everything could be done at the GtkMessageDialog level - the primary text is set in gtk_message_dialog_new() (the message_format param). I can scrub these changes off the patch if you disagree.
(In reply to Ernestas Kulik from comment #3) > Regarding the first two, there was some trailing whitespace I removed. The > latter two, I thought everything could be done at the GtkMessageDialog level > - the primary text is set in gtk_message_dialog_new() (the message_format > param). > > I can scrub these changes off the patch if you disagree. aah I see, the changes are nice. However, these changes need to be done separately. One patch per change. If not gets confusing why the commit is making unrelated changes regarding the issue the commit message or the bug report are describing. So the first patch would be to make them modal. The second patch can be to use the message_format param. The third patch can be to remove the trailing spaces. You can read more guidelines and tips for patches in https://wiki.gnome.org/Newcomers/CodeContributionWorkflow#Commit_guidelines
Created attachment 325306 [details] [review] [PATCH 1/3] mime-actions: make unhandled type dialogs modal
Created attachment 325307 [details] [review] [PATCH 2/3] mime-actions: don’t use g_object_set to set dialog text
Created attachment 325308 [details] [review] [PATCH 3/3] mime-actions: remove trailing whitespace
Review of attachment 325306 [details] [review]: LGTM
Review of attachment 325307 [details] [review]: Awesome! However there is something weird in your commit message subject. Seems your git client is messing up and adding some "header" there. Can you check the patch in raw text to ensure it's correct? You can click on "Review" here to check it as well. Basically I'm seeing this: Subject: [PATCH 2/3] =?UTF-8?q?mime-actions:=20don=E2=80=99t=20use=20g=5Fo?= =?UTF-8?q?bject=5Fset=20to=20set=20dialog=20text?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit
Review of attachment 325308 [details] [review]: Sure! I'm pondering at some point to run uncrustify on nautilus, but not sure if it's a good idea. I also hate these trailing spaces and the mix of code styles in nautilus...
Created attachment 325352 [details] [review] [PATCH 2/3] mime-actions: don't use g_object_set to set dialog text Unicode chars make the subject look screwy. In this case, the apostrophe.
(In reply to Carlos Soriano from comment #11) > Review of attachment 325308 [details] [review] [review]: > > Sure! > > I'm pondering at some point to run uncrustify on nautilus, but not sure if > it's a good idea. > I also hate these trailing spaces and the mix of code styles in nautilus... I am too on the fence about this. I’ve never used Uncrustify, but I’ll take a look.
(In reply to Ernestas Kulik from comment #12) > Created attachment 325352 [details] [review] [review] > [PATCH 2/3] mime-actions: don't use g_object_set to set dialog text > > Unicode chars make the subject look screwy. In this case, the apostrophe. Ah, I will double check on this to make sure I was not wrong about this.
(In reply to Ernestas Kulik from comment #13) > (In reply to Carlos Soriano from comment #11) > > Review of attachment 325308 [details] [review] [review] [review]: > > > > Sure! > > > > I'm pondering at some point to run uncrustify on nautilus, but not sure if > > it's a good idea. > > I also hate these trailing spaces and the mix of code styles in nautilus... > > I am too on the fence about this. I’ve never used Uncrustify, but I’ll take > a look. Sorry I was not clear, it was a random comment from my part, not need to use it yourself. It's just that I'm planning on do it on the whole project.
(In reply to Carlos Soriano from comment #15) > (In reply to Ernestas Kulik from comment #13) > > (In reply to Carlos Soriano from comment #11) > > > Review of attachment 325308 [details] [review] [review] [review] [review]: > > > > > > Sure! > > > > > > I'm pondering at some point to run uncrustify on nautilus, but not sure if > > > it's a good idea. > > > I also hate these trailing spaces and the mix of code styles in nautilus... > > > > I am too on the fence about this. I’ve never used Uncrustify, but I’ll take > > a look. > > Sorry I was not clear, it was a random comment from my part, not need to use > it yourself. It's just that I'm planning on do it on the whole project. Yeah, no, I realize. It was a random comment on my part as well. :) For the most part, Uncrustify works great, but I don’t think it can handle function parameter alignment “blockwise”.
Created attachment 325809 [details] [review] [PATCH 2/3] mime-actions: don't use g_object_set to set dialog text Messed up a little with the call. Setting the format string to the message itself is bad.
Review of attachment 325809 [details] [review]: Indeed, thanks!