Opened 2 years ago

Closed 2 years ago

#13463 closed enhancement (fixed)

adding a NeverDisableDefaultButton parameter to the BFilePanel constructor

Reported by: owenca Owned by: axeld
Priority: normal Milestone: Unscheduled
Component: Kits/libtracker.so Version: R1/Development
Keywords: BFilePanel, Tracker Cc: humdinger
Blocked By: Blocking:
Has a Patch: yes Platform: All

Description

Filer needs to have the default button in its file panel (activated by the Add button in the AutoFiler tab) enabled all the time. The only way to accomplish this is to append a default argument (for backward compatibility) to the current parameter list of the BFilePanel constructor. (Expander could have used it instead of adding the 3rd 'Select {parent folder}' button.)

If this is a good idea, I can work on a patch. The code change is expected to be minimal with no side effect.

Attachments (3)

enabled.png (27.8 KB ) - added by owenca 2 years ago.
the default Add button is always enabled (even if nothing is selected)
0004-BFilePanel-Add-setter-AlwaysEnableDefaultButton.patch (3.1 KB ) - added by owenca 2 years ago.
0004-Tracker-Always-enable-the-Open-button-for-B_DIRECTOR.patch (2.5 KB ) - added by owenca 2 years ago.

Download all attachments as: .zip

Change History (26)

comment:1 by axeld, 2 years ago

Adding arguments to a method, no matter if they have default values or not, will change their signature, and thus will break binary compatibility.

Can't you use a setter to accomplish this? And BTW, what do you need it for exactly?

comment:2 by owenca, 2 years ago

Adding a setter in the BFilePanel class instead? I didn't think of that, but it would be better than adding an argument to the constructor, I think. Should I try to see if adding a setter would work? Something like:

void BFilePanel::NeverDisableDefaultButton(bool = true);

We need it for https://github.com/HaikuArchives/Filer/issues/20.

in reply to:  1 comment:3 by humdinger, 2 years ago

Replying to axeld:

And BTW, what do you need it for exactly?

This is a solution for #8116. Instead of adding another button to the file panel to select the current folder, the standard select/add/open is simply always enabled. If no folder is selected, clicking the button takes the current folder. Not totally obvious, but beats the current drag of having to go up to the parent and selecting the folder there...

Note, this change is only for the flavour B_DIRECTORY_NODE of an B_OPEN_PANEL.

by owenca, 2 years ago

Attachment: enabled.png added

the default Add button is always enabled (even if nothing is selected)

comment:4 by owenca, 2 years ago

Has a Patch: set

comment:5 by owenca, 2 years ago

Yes, humdinger and I talked about it. I guess what he really wanted is a two-button solution with the default button always enabled, even if nothing is selected. (See the attached screenshot.)

I have attached the patch that created the screenshot. The setter's prototype is:

void BFilePanel::AlwaysEnableDefaultButton();

What do you think?

comment:6 by pulkomandy, 2 years ago

Do we really need the application to do this? Shouldn't this be automatic in B_DIRECTORY_NODE panels? If we make it a setter this way, we must then make sure that all applications use it, and if they forget to do so, it means the feature works in some applications, but not others.

Is there a reason to not want this enabled when using B_DIRECTORY_NODE? Even with applications customizing the file panel, I don't think there would be any bad consequences to that (we would have to check to make sure, after implementing it, of course).

Also, if there is already a ticket for things you are working on (#8116 in this case), do not create a new one, just continue the discussion there.

comment:7 by owenca, 2 years ago

I totally forgot about #8116 :(

Anyway, I thought there must be a reason why Tracker did it this way. It kind of made sense to disable the default Open button if nothing was selected.

I also thought Filer was one of the few apps that wanted to handle the default button differently. Hence the setter made sense...

comment:8 by humdinger, 2 years ago

I agree with PulkoMandy. Every B_OPEN_PANEL with a B_DIRECTORY_NODE flavour is used to select a folder. Every file panel shows the contents of a folder. If no folder is selected, the default selection should be the current folder itself.

At https://github.com/HaikuArchives/Filer/issues/20 we also discussed showing the selected folder name as the button label. That would make the current selection obvious, but as Owen pointed out, that would lead to jumping buttons, because of changing folder name length.

comment:9 by owenca, 2 years ago

Ok. I will work on a patch that does what PulkoMandy and humdinger asked.

comment:10 by axeld, 2 years ago

I think the solution is "okay" (not great, but acceptable) for B_DIRECTORY_NODE, but once a file panel supports both, files and directories, it's not that "okay" anymore IMO. It's something non-obvious that could actually be done by default, though.

Other than that, I'm still in favor of what I said in ticket:8116#comment:7.

comment:11 by owenca, 2 years ago

So what to do exactly now? Use the setter in the attached patch, add a flag, or keep the default button enabled in B_OPEN_PANEL for B_DIRECTORY_NODE?

comment:12 by axeld, 2 years ago

As a start, I'd keep "open" always enabled on directory mode. However, the long term solution should, IMO, add another button by default, iff the software was built for Haiku.

in reply to:  12 comment:13 by owenca, 2 years ago

Just want to make sure, you mean to keep the default button always enabled only for B_OPEN_PANEL mode (so no B_SAVE_PANEL mode) with B_DIRECTORY_NODE flavor? What if the node flavors also include B_FILE_NODE and/or B_SYMLINK_NODE in addition to B_DIRECTORY_NODE?

BTW, in #8116, you and PulkoMandy talked about a possible solution of adding a flag, is it similar to adding a setter?

in reply to:  10 comment:14 by humdinger, 2 years ago

Replying to axeld:

I think the solution is "okay" (not great, but acceptable) for B_DIRECTORY_NODE, but once a file panel supports both, files and directories, it's not that "okay" anymore IMO.

I've been thinking...why should the proposed behaviour be restricted to pure B_DIRECTORY_NODE flavour? Even if you mix B_FILE_NODE and/or B_SYMLINK_NODE to it, taking the current folder if nothing's selected still seems like a good idea and also consistent.

comment:15 by owenca, 2 years ago

The 3rd button 'Select current folder' in Expander is redundant now.

comment:16 by owenca, 2 years ago

According to the Confirmable Node Flavors section at https://www.haiku-os.org/legacy-docs/bebook/BFilePanel_Overview.html, it seems that keeping the Open button enabled makes sense only for B_DIRECTORY_NODE.

Last edited 2 years ago by owenca (previous) (diff)

in reply to:  16 comment:17 by humdinger, 2 years ago

Replying to owenca:

According to the Confirmable Node Flavors section at https://www.haiku-os.org/legacy-docs/bebook/BFilePanel_Overview.html, it seems that keeping the Open button enabled makes sense only for B_DIRECTORY_NODE.

I don't really read that there. IMO, as soon as the B_DIRECTORY_NODE flavour is in the mix, it makes sense to accept the current directory if nothing is selected.

Do you mean from the BeBook:

"If the setting includes B_FILE_NODE and the user selects and confirms a file or a symlink to a file, the file (or symlink) is delivered to your target. If it doesn't include B_FILE_NODE and the user selects a file (or symlink to a file), the Open button is disabled."

I don't think the last sentence applies any more. If there's no "B_FILE_NODE" flavour included, the file panel doesn't even show files, only directories.

comment:18 by axeld, 2 years ago

Why only for open panels, and why not when both, file, and directories are requested? Also, the method should be called IsOpenButtonAlwaysEnabled().

comment:19 by owenca, 2 years ago

humdinger, thanks for helping me understand it!

axeld, I think we should leave save panels alone because

  1. the node flavors only apply to open panels, and
  2. the handling of enabling/disabling the Save button seems to do what it's supposed to do.

(If a folder is selected in a save panel, the Save button's label changes to Open, and it will enter the selected folder. Making it always enabled would just allow re-entering the folder it's already in because the popup menu field in the upper left corner always shows the name of the current panel directory.)

Please see the new patch with the function name change. Now the default button in an Open panel is always enabled if the B_DIRECTORY_NODE flag is set, regardless if other flags are set.

What shall we do about the 'Select {current folder}' button in Expander? It would do exactly the same thing as the default Select button when nothing is selected.

comment:20 by axeld, 2 years ago

Ah, now I understand; yes, then the patch looks almost good. My only complaint would be that lines 808/809 are necessary, but that's probably unavoidable :-)

I would keep the button in Expander as is for now; as I said, I would prefer such a button by default in Haiku for B_DIRECTORY_NODE panels, anyway.

in reply to:  20 comment:21 by owenca, 2 years ago

Replying to axeld:

Ah, now I understand; yes, then the patch looks almost good. My only complaint would be that lines 808/809 are necessary, but that's probably unavoidable :-)

I reviewed the code again, and you are right that lines 809/810 are necessary. Otherwise, the Open button of Pe, StyleEdit, and the like would be enabled when the file panel is first shown with nothing selected. The button would then try to "Open" the current panel directory.

Last edited 2 years ago by owenca (previous) (diff)

comment:22 by humdinger, 2 years ago

Owner: changed from nobody to axeld
Status: newassigned

@axeld: since you gave your comments on the patch before, I'll assign the ticket to you. You can get rid of it quickly by applying the patch... :)

comment:23 by axeld, 2 years ago

Resolution: fixed
Status: assignedclosed

Applied in hrev51155, thanks!

Note: See TracTickets for help on using tickets.