Closed Bug 267219 Opened 17 years ago Closed 16 years ago
[BEOS] Selecting a directory fails and is uninituitive
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:
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: jag → beos
Component: XP Toolkit/Widgets → GFX: BeOS
QA Contact: Niels.Reedijk → timeless
niels: please handle the cases when new fails. (believe me, this does happen)
Status: UNCONFIRMED → NEW
Ever confirmed: true
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?
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.
Remove debug output and check if new fails.
Attachment #164234 - Attachment is obsolete: true
Comment on attachment 165247 [details] [diff] [review] Updated patch Obsolete patch
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.
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)
(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.
Only obstacle i see here, is i18n. Inability to autochange button label according to user language. But that's another problem.
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.
(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.
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.
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.
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.
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 on attachment 165248 [details] [diff] [review] Move MSG_DIRECTORY to implementation email@example.com 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 on attachment 165248 [details] [diff] [review] Move MSG_DIRECTORY to implementation sr
Attachment #165248 - Flags: superreview?(sergei_d) → superreview+
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 on attachment 165248 [details] [diff] [review] Move MSG_DIRECTORY to implementation a=asa for trunk checkin.
Attachment #165248 - Flags: approval1.8a5? → approval1.8a5+
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-
Timeless: could you check this in?
fix checked in
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.