[BEOS] Selecting a directory fails and is uninituitive

RESOLVED FIXED

Status

Core Graveyard
GFX: BeOS
RESOLVED FIXED
13 years ago
8 years ago

People

(Reporter: Niels Reedijk, Assigned: Niels Reedijk)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

13 years ago
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:
(Assignee)

Updated

13 years ago
Blocks: 266252
(Assignee)

Comment 1

13 years ago
Created attachment 164234 [details] [diff] [review]
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
(Assignee)

Updated

13 years ago
QA Contact: jrgmorrison → Niels.Reedijk

Updated

13 years ago
Assignee: jag → beos
Component: XP Toolkit/Widgets → GFX: BeOS
QA Contact: Niels.Reedijk → timeless

Comment 2

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

Comment 3

13 years ago
confirming
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 4

13 years ago
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? 
(Assignee)

Comment 5

13 years ago
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.
(Assignee)

Comment 6

13 years ago
Created attachment 165247 [details] [diff] [review]
Updated patch

Remove debug output and check if new fails.
Attachment #164234 - Attachment is obsolete: true
(Assignee)

Updated

13 years ago
Attachment #165247 - Flags: superreview?(sergei_d)
Attachment #165247 - Flags: review?(thesuckiestemail)
(Assignee)

Comment 7

13 years ago
Comment on attachment 165247 [details] [diff] [review]
Updated patch

Obsolete patch
Attachment #165247 - Attachment is obsolete: true
Attachment #165247 - Flags: superreview?(sergei_d)
Attachment #165247 - Flags: review?(thesuckiestemail)
(Assignee)

Comment 8

13 years ago
Created attachment 165248 [details] [diff] [review]
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.
(Assignee)

Updated

13 years ago
Attachment #165248 - Flags: superreview?(sergei_d)
Attachment #165248 - Flags: review?(thesuckiestemail)

Comment 9

13 years ago
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)
(Assignee)

Comment 10

13 years ago
(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

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

Comment 12

13 years ago
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.
(Assignee)

Comment 13

13 years ago
(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

13 years ago
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.
(Assignee)

Comment 15

13 years ago
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

13 years ago
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

13 years ago
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

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

r=thesuckiestemail@yahoo.se

Looks ok, works ok. Although I don't like the prefs for selecting
download-folder under BeOS,
 it seems to be quite flawed
Attachment #165248 - Flags: review?(thesuckiestemail) → review+

Comment 19

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

sr
Attachment #165248 - Flags: superreview?(sergei_d) → superreview+
(Assignee)

Comment 20

13 years ago
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.
Attachment #165248 - Flags: approval1.8a5?

Comment 21

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

a=asa for trunk checkin.
Attachment #165248 - Flags: approval1.8a5? → approval1.8a5+
(Assignee)

Updated

13 years ago
Attachment #165248 - Flags: approval-aviary?

Comment 22

13 years ago
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.
Attachment #165248 - Flags: approval-aviary? → approval-aviary-
(Assignee)

Updated

13 years ago
Assignee: beos → Niels.Reedijk
(Assignee)

Updated

13 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 23

13 years ago
Timeless: could you check this in?

Comment 24

13 years ago
fix checked in
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.