267219 - [BEOS] Selecting a directory fails and is uninituitive

Status: RESOLVED FIXED

(Core Graveyard :: GFX: BeOS, defect)

Description

[Niels Reedijk]

User-Agent:       Mozilla/5.0 (BeOS; U; BeOS BePC; en-US; rv:1.7.3) Gecko/20041028 Firefox/1.0RC1
Build Identifier: Mozilla/5.0 (BeOS; U; BeOS BePC; en-US; rv:1.7.3) Gecko/20041028 Firefox/1.0RC1

When using a directory selector, the directory isn't stored when it is selected.
Furthermore, opening a directory is uninituitive. 

This bug is the beos cousin of bug 266252 .

Reproducible: Always
Steps to Reproduce:

Comment 1

[Niels Reedijk]

Attachment: Improves directory selection and fixes the actual storage of the selected new dir.

This is a first patch of mine (and automagically a first try at this bug).
Check out it's brother bug report for the complete problem description. 

Only thing I'm not completely sure about is where to put the MSG_DIRECTORY
message definition. Any hints? If that question is answered, I'll put it up for
review.

Niels

Comment 2

[timeless]

niels: please handle the cases when new fails. (believe me, this does happen)

Comment 3

[tqh]

confirming

Comment 4

[Niels Reedijk]

Where should I put:
 const uint32 MSG_DIRECTORY = 'mDIR';
Can I just put it in the implementation? It's used only there. Or should I
#define it? 

Comment 5

[Niels Reedijk]

BTW, at the moment this patch contains a buffer overflow when the name and the
label are larger than 50 characters. I'll have that fixed as well.

Comment 6

[Niels Reedijk]

Attachment: Updated patch

Remove debug output and check if new fails.

Comment 7

[Niels Reedijk]

Comment on attachment 165247 [details] [diff] [review]
Updated patch

Obsolete patch

Comment 8

[Niels Reedijk]

Attachment: Move MSG_DIRECTORY to implementation

I moved the MSG_DIRECTORY definiton to the implementation. I wonder whether
this is te right place to put it, and if the way of defining it (as a const
uint32) is good as well. I haven't found any other messages in the source yet,
so I'm wondering what I should do with this one.

Comment 9

[Sergei Dolgov]

Niels, for future:
1)Ask for superreview when review is already granted.
2)In this case, as we change files in BeOS-only folder, we don't need superreview.

Also, about patch itself.
Isn't more reasonable to use B_PATH_NAME_LENGTH instead B_FILE_NAME_LENGTH ?
(I cannot test/inestigate real code at the moment, so asking)

Comment 10

[Niels Reedijk]

(In reply to comment #9)
> Niels, for future:
> 1)Ask for superreview when review is already granted.
> 2)In this case, as we change files in BeOS-only folder, we don't need superreview.

I'll keep that in mind.

> Also, about patch itself.
> Isn't more reasonable to use B_PATH_NAME_LENGTH instead B_FILE_NAME_LENGTH ?
> (I cannot test/inestigate real code at the moment, so asking)

No, it's not. The string is 10 bytes long (including the \0), plus the maximum
length of a filename. There isn't any path involved, so this one is more logical.

Comment 11

[Sergei Dolgov]

Only obstacle i see here, is i18n. Inability to autochange button label according 
 to user language. But that's another problem.

Comment 12

[Cian Duffy]

i18n doesn't work on BeOS anyway, unless you perform serious voodoo, so breaking
it even more is not an issue. No XPI the Irish Localisation Project has ever
generated for Firefox has worked.

Comment 13

[Niels Reedijk]

(In reply to comment #12)
> i18n doesn't work on BeOS anyway, unless you perform serious voodoo, so breaking
> it even more is not an issue. No XPI the Irish Localisation Project has ever
> generated for Firefox has worked.

Furthermore, the dialog is in English (for another three years anyway, minimum).
So it would be quite strange to have an english dialog with a "<map> selecteren"
button.

Comment 14

[Sergei Dolgov]

As i said, this problem isn't important, just had wish to notice that witj
localized Tracker version FilePanel will/may look bit strange.
And anyway, current Mozilla/Firefox nsFilePicker BeOS implementation is real crap. 

But i really prefer don't see here such incompetent  "arguments" as MYOB's
comment. It can compromise even really perfect patch and even occasionally
trigger on veto from mozilla VIPs.

Comment 15

[Niels Reedijk]

I'll try looking at the i18n functions that are available. If it is doable, then
I'll update the patch. The problem is, that we'll have to work in unicode, and I
don't know how that can be transfered to a bbutton.

Comment 16

[Sergei Dolgov]

Don't tinker with it now.
See, this nsFilePicker is messy object - half-native.
And we miss in BeOS atm established localization concept.
At least until Haiku is out and has unified with Zeta localization API.

And mozilla generic localization works on XUL level, which is also very hard to
connect to our case. Workaround may be bitmap-buton but i'm not aware about any
established icon for "Select" action.

So if people like that option and tqh will put review at that, my sr comes
automatically.

Comment 17

[tqh]

I agree with fyysik's comments, but that maybe should be handled in a future
bug. this patch looks like a nice improvement.

I've looked at the code and it looks ok, but I want to setup a tree and test it
too before I review it. Hopefully I will be able to do it today or tomorrow (19/11).

Comment 18

[tqh]

Comment on attachment 165248 [details] [diff] [review]
Move MSG_DIRECTORY to implementation

r=[e-mail]

Looks ok, works ok. Although I don't like the prefs for selecting
download-folder under BeOS,
 it seems to be quite flawed

Comment 19

[Sergei Dolgov]

Comment on attachment 165248 [details] [diff] [review]
Move MSG_DIRECTORY to implementation

sr

Comment 20

[Niels Reedijk]

Comment on attachment 165248 [details] [diff] [review]
Move MSG_DIRECTORY to implementation

Requesting aproval as trunk is frozen. This touches BeOS only-code, and will
not affect any platforms in any way.

Comment 21

[Asa Dotzler [:asa]]

Comment on attachment 165248 [details] [diff] [review]
Move MSG_DIRECTORY to implementation

a=asa for trunk checkin.

Comment 22

[Asa Dotzler [:asa]]

Comment on attachment 165248 [details] [diff] [review]
Move MSG_DIRECTORY to implementation

1.0 has shipped and we're looking to the trunk for future Firefox releases.

Comment 23

[Niels Reedijk]

Timeless: could you check this in?

Comment 24

[Bernd]

fix checked in