Skip to content

commander: small improvement on listing and panel width #1394

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

icy
Copy link

@icy icy commented Dec 14, 2024

Motivation

When working with different directories that have some common files that have long full path names, it's hard to find the right file from the list. This is because the panel's width is 500 and the full path may not be shown completely within the panel as below

<-------- visible to  user --------> <----- invisible to users ------------------->

/-----------------------------------\
| /foo/bar/directory/with/very/long-|/path/name1/sample.txt
| Files -> Recent Files -> /foo/bar/|directory/with/very/long-/path1/name/sample.txt
| /foo/bar/directory/with/very/long-|/path/name2/sample.txt
| Files -> Recent Files -> /foo/bar/|directory/with/very/long-/path2/name/sample.txt
\-----------------------------------/

In this illustration, when looking up sample.txt, the user has no idea about the directory information (both path/name1 and path/name2 do matching)

The patches

  • The panel's width is one half of the width of the Geany window when this amount is greater than or equals to 250
  • Otherwise the panel's width defaults to the width of the Geany window
  • For labels, don't display full names; only display the parent directory's basename and the file name

The previous illustration now becomes

/-----------------------------------\
| name1/sample.txt                  |
| Files -> Recent Files -> /foo/bar/|directory/with/very/long-/path/name1/sample.txt
| name2/sample.txt                  |
| Files -> Recent Files -> /foo/bar/|directory/with/very/long-/path/name2/sample.txt
\-----------------------------------/

and this helps user to quickly find the right files.

Of course this doesn't solve all edge cases when there are a few more levels of duplication in full path names.

@eht16
Copy link
Member

eht16 commented Jan 4, 2025

Could you try to remove the unnecessary white space changes? This would make the diff easier to read and less noisy (I'm aware I can disable showing whitespace changes but they are still there).

Or we manage to convince @b4n to accept stripping trailing whitespace :).

@icy
Copy link
Author

icy commented Jan 4, 2025

Hi @eht16 ,

Thanks for your suggestion. I will update the patch with spaces removed.

@icy
Copy link
Author

icy commented Jan 5, 2025

Hi @eht16 ,

Thanks for your suggestion. I will update the patch with spaces removed.

I am using Geany to edit the file, and one of the plugin/functions of the editor has edited the spaces without my intention. I have tried to squash the commits and give another try today.

Update: I found the cause of my problem: geany/geany#3942 :D

When working  with different directories that have some common files
that have long full path names, it's hard to find the right file from
the list. This is because the panel's width is 500 and the full path may
not be shown completely within the panel as below

```
<-------- visible to  user --------> <----- invisible to users ------------------->

/-----------------------------------\
| /foo/bar/directory/with/very/long-|/path/name1/sample.txt
| Files -> Recent Files -> /foo/bar/|directory/with/very/long-/path1/name/sample.txt
| /foo/bar/directory/with/very/long-|/path/name2/sample.txt
| Files -> Recent Files -> /foo/bar/|directory/with/very/long-/path2/name/sample.txt
\-----------------------------------/

```

In this illustration, when  looking up  `sample.txt`, the user has no
idea about the directory  information (both `path/name1` and
`path/name2` do matching)

- [ ] The panel's width is  one half of the width of the Geany window
      when this amount is greater than or equals to 250
- [ ] Otherwise the panel's width defaults to the width of the Geany window
- [ ] For labels, don't display  full names; only display the parent
      directory's basename and the file name

The previous illustration now becomes

```
/-----------------------------------\
| name1/sample.txt                  |
| Files -> Recent Files -> /foo/bar/|directory/with/very/long-/path/name1/sample.txt
| name2/sample.txt                  |
| Files -> Recent Files -> /foo/bar/|directory/with/very/long-/path/name2/sample.txt
\-----------------------------------/
```

and this helps user to quickly find the right files.

Of course this doesn't solve all edge cases when there are a few more
levels of  duplication in full path names.
@icy
Copy link
Author

icy commented Jan 5, 2025

Could you try to remove the unnecessary white space changes? This would make the diff easier to read and less noisy (I'm aware I can disable showing whitespace changes but they are still there).

Or we manage to convince @b4n to accept stripping trailing whitespace :).

Hi @eht16,

I've updated my pull request and I think it's easier to review now. Please have a look when you've time.

Thank you very much.

Copy link
Member

@b4n b4n left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not against a way to better discriminate between similar-looking paths, but I think it needs more work not to be as specific to your use case where the parent's directory name is enough.

Also, please split changes into individual commits (e.g. the change to window size seem unrelated), and make sure the rationale behind each change is clear -- for example, the one behind the window size isn't clear to me.

Comment on lines -348 to +357
gchar *label = g_markup_printf_escaped ("<big>%s</big>", item_label);
gchar *basename = g_path_get_basename (item_label);
gchar *dirname = g_path_get_dirname (item_label);
gchar *dirname_basename = g_path_get_basename(dirname);
gchar *label;

if (g_strcmp0(".", dirname_basename) == 0) {
label = g_markup_printf_escaped ("<big>%s</big>", basename);
} else {
label = g_markup_printf_escaped ("<big>%s/%s</big>", dirname_basename, basename);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems overly fragile to me, as menu item labels can contain pretty much anything, including /s.

Take for example Edit → Insert Date → yyyy/mm/dd: with this change, it appears as mm/dd.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that may be a problem, and in that case, it's also possible to see all details in the smaller part of the menu item ( Edit → Insert Date → yyyy/mm/dd).

Comment on lines +425 to +429
gchar *dirname = g_path_get_dirname (DOC_FILENAME (documents[i]));
gchar *dirname_basename = g_path_get_basename (dirname);
gchar *label = g_markup_printf_escaped ("<big>%s/%s</big>\n"
"<small><i>%s</i></small>",
dirname_basename,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here it's not as problematic as those are paths. However, the dirname is not necessarily helping much as it might very well still be the same (e.g. if there are foo/src/main.c and bar/src/main.c, this change won't help).

If this is really an issue you encounter, I suggest investigating a more clever solution, possibly verifying there are effectively duplicates, and then compute the difference between the paths in order to show parts that are actually relevant. IIRC there's something like that in Geany's got to popup you might get inspiration from.

Once that's dealt with I wonder what the best visual layout would be… maybe <big>$basename</big> <small>($path_diff)</small>\n<small><i>$path</i></small>? Not tested though, but I think preserving focus on the filename, or actually what's really relevant in it, is important. So maybe even highlight the different parts?

Copy link
Author

@icy icy Feb 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that your suggestion would be better and would address all cases well.

I was actually thinking if I could do some computing for all items before getting the best display option. However, I think that would require more resources, and I don't think it's not worth trading. I was really getting some hard time using our plugin, and the best idea I've been able to share, is this (wip) pull request.

I will spend a little more time on this, at least for my own usage. If you think there should be more efficient way to address the problem, I can help to open an issue and hopefully someone from the community can help.

Thank you a lot for your time.

Comment on lines +682 to +684
if (window_width >= 500) {
window_width = window_width / 2;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is 500 really too big? Otherwise if the goal is to make it bigger, then the threshold should be whatever makes it larger than 500 (math suggests it'd be 1000).

Also note that this is not dynamic during a Geany session, it's only the default size when opening the panel for the first time, if you change the Geany window size later it won't affect this. This is not necessarily an issue though.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is 500 really too big? Otherwise if the goal is to make it bigger, then the threshold should be whatever makes it larger than 500 (math suggests it'd be 1000).

When the main window has some moderate size , I want(ed) the commander's panel has relatively smaller size. When the main window is too small (< 500), I want to see the commander of the same size (otherwise it's too small to show any thing if I get it right.)

I admit my calculation is bad. It's better that I used some min() function instead.

Also note that this is not dynamic during a Geany session, it's only the default size when opening the panel for the first time, if you change the Geany window size later it won't affect this. This is not necessarily an issue though

I've also noticed that, thanks for your confirmation.

@@ -804,4 +828,4 @@ void
plugin_help (void)
{
utils_open_browser (DOCDIR "/" PLUGIN "/README");
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please bring the trailing newline back 🙂

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure I will do that. This would be a bug somewhere in (my) Geany system.

OT: I was into this issue #1394 (comment), now I have two options at project level (Ensure new line at file end [checked] + Strip trailing spaces and tabs [unchecked]). Somehow only one of them works as expected :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants