Merge lp:~jmiguelbenitez/pantheon-files/fix-1082680 into lp:~elementary-apps/pantheon-files/trunk

[José M. Benítez]

Files in columns view now open with single click properly to fix bug #1082680

Fix for bug #1082680 (open file with one click doesn't work in column view).

[Cody Garver (codygarver)]

Right clicking still opens things in Columns mode. Maybe that bug was not intended to be attached to this branch?

[José M. Benítez (jmiguelbenitez)]

You are absolutely right, my man!

My mistake. I already removed the link between bug and branch O:)

Thanks!

2014/1/19 Cody Garver <email address hidden>

> Right clicking still opens things in Columns mode. Maybe that bug was not
> intended to be attached to this branch?
> --
>
> https://code.launchpad.net/~jmiguelbenitez/pantheon-files/fix-1082680/+merge/202213
> You are the owner of lp:~jmiguelbenitez/pantheon-files/fix-1082680.
>

[lp:~jmiguelbenitez/pantheon-files/fix-1082680]

1403. By José M. Benítez on 2014-01-19

Open regular files in column view with a click only if the setting is on

[Jeremy Wootten (jeremywootten)]

If you select two files using Ctrl-LeftButton and then deselect one of them using Ctrl-LeftButton then the remaining selected file is opened. This does not occur in icon view and list view and is not a desirable behaviour.

There is potential that if the fix to bug 1028637 is merged (which automatically selects files that are dropped or pasted into a view) then dropping or pasting a single file into a column view would cause that file to be opened, though I haven't checked this yet.

The file should only open when the file alone becomes selected from an unselected state only by virtue of a single mouse click when the single-click option is set.

review: Needs Fixing

[José M. Benítez (jmiguelbenitez)]

I don't want to make any excuses here, but that faulty behaviour is not introduced by the new code: I'm seeing now it was already there for directories. I understand though that it's much more annoying (and wrong, of course) when selecting files.

Anyway, the point I'm trying to make here is whether I should file another bug and solve it before this one. I did it already with a similar side effect (bug #1270566). Well, I mean I filed a bug, but I did not solve it O:)

Regarding the fix to bug #1028637, it doesn't seem it's going to have an affect here, but I've to look at it more closely.

[Jeremy Wootten (jeremywootten)]

I am not an authority on this but if the new branch exposes new bug or
magnifies an existing bug that:

1) involves significant changes to parts of the code that is not already
being changed by the new branch and
2) is a bug in its own right - i.e. would be worth fixing whether or not
the new branch is accepted

then I would say that it is best to create another branch that fixes that
bug (or file a bug and wait for some else to fix it!) and make the merging
of the first branch depend on the prior merging of the branch that fixes
that bug.

Having said that, I just created a branch enabling smooth scrolling that
exposed a deficiency in related code that had no effect whatsoever unless
smooth-scrolling was enabled but seriously degraded the performance of
zooming when it was. As it was comparatively easily fixed and was closely
related to smooth scroll event handling, I fixed it in the same branch.
I'll just have to see if anyone objects ...

When in doubt, create a new branch.

On Sun, Feb 23, 2014 at 11:19 AM, José M. Benítez
<email address hidden>wrote:

> I don't want to make any excuses here, but that faulty behaviour is not
> introduced by the new code: I'm seeing now it was already there for
> directories. I understand though that it's much more annoying (and wrong,
> of course) when selecting files.
>
> Anyway, the point I'm trying to make here is whether I should file another
> bug and solve it before this one. I did it already with a similar side
> effect (bug #1270566). Well, I mean I filed a bug, but I did not solve it
> O:)
>
> Regarding the fix to bug #1028637, it doesn't seem it's going to have an
> affect here, but I've to look at it more closely.
>
> --
>
> https://code.launchpad.net/~jmiguelbenitez/pantheon-files/fix-1082680/+merge/202213
> You are reviewing the proposed merge of
> lp:~jmiguelbenitez/pantheon-files/fix-1082680 into lp:pantheon-files.
>

[José M. Benítez (jmiguelbenitez)]

Thanks once more, Jeremy.

I'll see whether it's easy to solve it all in this branch. If the thing goes out of hand, I'll create another one.

[lp:~jmiguelbenitez/pantheon-files/fix-1082680]

1404. By José M. Benítez on 2014-02-25

Fixed bug by which the last selected file after deselecting all others was mistakenly open.

[José M. Benítez (jmiguelbenitez)]

I think now it's ok, isn't it?

There's still something going on with Ctrl-selecting one element in a brand new slot, but IMHO it has nothing to do with this bug.

[Jeremy Wootten (jeremywootten)]

Sorry I have not had a chance to pursue this for a while - I have been
distracted by trying to construct a build environment for the latest trunk
version of pantheon-files - without success so far! The switch to gee-0.8
caused the problem. As you have not merged that revision I should be able
to build your branch still.

On Tue, Feb 25, 2014 at 12:07 AM, José M. Benítez
<email address hidden>wrote:

> I think now it's ok, isn't it?
>
> There's still something going on with Ctrl-selecting one element in a
> brand new slot, but IMHO it has nothing to do with this bug.
>
> --
>
> https://code.launchpad.net/~jmiguelbenitez/pantheon-files/fix-1082680/+merge/202213
> You are reviewing the proposed merge of
> lp:~jmiguelbenitez/pantheon-files/fix-1082680 into lp:pantheon-files.
>

[Jeremy Wootten (jeremywootten)]

I now have a working Isis build environment but in the process lost my Luna
build environment. So I merged the current trunk with your branch and
tested that (there were no conflicts). You will have to merge changes from
trunk at some point anyway. However, be aware that when you do this you
may have to update your build environment.

Your latest revision opens the file with a fast right-click (so the context
menu opens after the button release). This can be fixed (for example -
there may be better ways) by changing the button_release callback code to:

    if (view->details->pressed_button == event->button &&
view->details->pressed_button != -1)
    {
        /* don't do anything if any modifier key is active */
        if ((event->state & gtk_accelerator_get_default_mod_mask ()) == 0
         && view->details->pressed_button != 3)
        {
            view->details->updates_frozen = FALSE;
            selection = gtk_tree_view_get_selection (tree_view);
            list_selection_changed_callback (selection, view);
        }
        /* reset the pressed_button state */
        view->details->pressed_button = -1;
    }

Apart from that it seems to work OK.

On Tue, Feb 25, 2014 at 12:07 AM, José M. Benítez
<email address hidden>wrote:

> I think now it's ok, isn't it?
>
> There's still something going on with Ctrl-selecting one element in a
> brand new slot, but IMHO it has nothing to do with this bug.
>
> --
>
> https://code.launchpad.net/~jmiguelbenitez/pantheon-files/fix-1082680/+merge/202213
> You are reviewing the proposed merge of
> lp:~jmiguelbenitez/pantheon-files/fix-1082680 into lp:pantheon-files.
>

[lp:~jmiguelbenitez/pantheon-files/fix-1082680]

1405. By José M. Benítez on 2014-03-16

Modified code so that right clicks do not open files in column view mode.

[José M. Benítez (jmiguelbenitez)]

Thanks, Jerermy, and sorry for the late reply.

I think the right-click thing is solved now (I haven't used your suggestion because of an item selection issue). Anyway, GtkTreeView has got an "activate-on-single-click" signal since 3.8 which IMHO would make all this much cleaner, but I'm not sure if all other devs are on Isis already.

Regarding that, you're right, I should update my build environment. It's just that I'm waiting for my new laptop to do it.

[Jeremy Wootten (jeremywootten)]

That's fixed it, good work!

I now approve.

review: Approve

[Cody Garver (codygarver)]

Use the Gtk 3.8 signal, it's safe to use up to Gtk 3.10 at this point. And probably 3.12 soon.

[José M. Benítez (jmiguelbenitez)]

Ok, great. However, if you both agree, I'd merge this branch as it is and leave the new code using the signal for another branch (I will file the request myself).

It's just that I find the column view behaviour a bit inconsistent, as it accepts single-clicks no matter the setting. So I would like to make all that clear before I get the things wrong.

=== modified file 'src/fm-columns-view.c'
--- src/fm-columns-view.c	2014-02-17 00:56:55 +0000
+++ src/fm-columns-view.c	2014-03-16 10:02:05 +0000
@@ -95,12 +95,6 @@
 }
 
 static void
-show_selected_files (GOFFile *file)
-{
-    g_message ("selected: %s\n", g_file_info_get_name (file->info));
-}
-
-static void
 list_selection_changed_callback (GtkTreeSelection *selection, FMColumnsView *view)
 {
     GOFFile *file;
@@ -109,7 +103,6 @@
     if (view->details->selection != NULL)
         gof_file_list_free (view->details->selection);
     view->details->selection = get_selection (view);
-    //show_selected_files (file);
 
     if (FM_DIRECTORY_VIEW (view)->updates_frozen)
         return;
@@ -123,7 +116,7 @@
 
     if (view->details->selection == NULL)
         return;
-    /* dont show preview or load directory if we got more than 1 element selected */
+    /* don't show preview or load directory if we got more than 1 element selected */
     if (view->details->selection->next)
         return;
     if (view->details->updates_frozen)
@@ -134,7 +127,12 @@
         fm_directory_view_column_add_location (FM_DIRECTORY_VIEW (view), file->location);
         /* give back the focus to the active slot */
         gtk_widget_grab_focus (GTK_WIDGET (view));
-    }
+    } else if (view->details->pressed_button == 1 && g_settings_get_boolean (settings, "single-click"))
+        /* Open a file provided that it's been selected with the mouse (left
+         * clicks if single-click mode is activated): we don't want files to be
+         * open when changing selection with the keyboard.
+         */
+        fm_directory_view_activate_selected_items (FM_DIRECTORY_VIEW (view), MARLIN_WINDOW_OPEN_FLAG_DEFAULT);
 }
 
 static void
@@ -347,6 +345,7 @@
         return FALSE;
 
     view->details->pressed_button = -1;
+
     /* we unselect all selected items if the user clicks on an empty
      * area of the treeview and no modifier key is active.
      */
@@ -357,7 +356,7 @@
         gtk_tree_selection_unselect_all (selection);
     }
 
-    selection = gtk_tree_view_get_selection (tree_view);
+    /* left clicks */
     if (event->type == GDK_BUTTON_PRESS && event->button == 1) {
         /* save last pressed button */
         if (view->details->pressed_button < 0) {
@@ -366,6 +365,7 @@
         }
     }
 
+    selection = gtk_tree_view_get_selection (tree_view);
     /* open the context menu on right clicks */
     if (event->type == GDK_BUTTON_PRESS && event->button == 3)
     {
@@ -373,6 +373,7 @@
             view->details->pressed_button = event->button;
             view->details->updates_frozen = TRUE;
         }
+
         if (gtk_tree_view_get_path_at_pos (tree_view, event->x, event->y, &path, NULL, NULL, NULL))
         {
             /* select the path on which the user clicked if not selected yet */
@@ -400,7 +401,8 @@
 
         return TRUE;
     }
-    else if ((event->type == GDK_BUTTON_PRESS || event->type == GDK_2BUTTON_PRESS) && event->button == 2)
+
+    if ((event->type == GDK_BUTTON_PRESS || event->type == GDK_2BUTTON_PRESS) && event->button == 2)
     {
         /* determine the path to the item that was middle-clicked */
         if (gtk_tree_view_get_path_at_pos (tree_view, event->x, event->y, &path, NULL, NULL, NULL))
@@ -458,15 +460,18 @@
     g_message ("%s", G_STRFUNC);
     if (view->details->pressed_button == event->button && view->details->pressed_button != -1)
     {
-        view->details->updates_frozen = FALSE;
-        selection = gtk_tree_view_get_selection (tree_view);
-        list_selection_changed_callback (selection, view);
+        /* don't do anything if any modifier key is active */
+        if ((event->state & gtk_accelerator_get_default_mod_mask ()) == 0)
+        {
+            view->details->updates_frozen = FALSE;
+            selection = gtk_tree_view_get_selection (tree_view);
+            list_selection_changed_callback (selection, view);
+        }
 
         /* reset the pressed_button state */
         view->details->pressed_button = -1;
     }
 
-
     return TRUE;
 }