Opened 11 years ago

Last modified 2 years ago

#2185 in-progress enhancement

Scan the source tree and use find_directory() where appropriate (no hard-coded paths anymore)

Reported by: kaoutsis Owned by: stippi
Priority: normal Milestone: Unscheduled
Component: - General Version: R1/pre-alpha1
Keywords: Cc: leavengood@…
Blocked By: Blocking:
Has a Patch: no Platform: All

Description

Attachments (14)

devfs.cpp.diff (2.2 KB) - added by kaoutsis 11 years ago.
team.cpp.diff (1.4 KB) - added by kaoutsis 11 years ago.
HWindow.cpp-v2.diff (15.2 KB) - added by kaoutsis 11 years ago.
with style changes
HWindow.cpp.diff (1.4 KB) - added by kaoutsis 11 years ago.
only with the find_directory changes
module.cpp.diff (1.2 KB) - added by kaoutsis 11 years ago.
TrackerInitialState.cpp.diff (1.1 KB) - added by kaoutsis 11 years ago.
src_servers_app_FontManager.cpp.diff (1.2 KB) - added by kaoutsis 11 years ago.
src_bin_desklink_desklink.cpp.diff (3.8 KB) - added by kaoutsis 11 years ago.
src_system_kernel_main.cpp.diff (19.3 KB) - added by kaoutsis 11 years ago.
src_preferences_screensaver_ScreenSaverApp.cpp.diff (7.4 KB) - added by kaoutsis 11 years ago.
src_preferences_screensaver_ScreenSaverApp.cpp.2.diff (9.4 KB) - added by kaoutsis 11 years ago.
src_servers_registrar_TRoster.cpp.diff (2.3 KB) - added by kaoutsis 11 years ago.
src_servers_debug_DebugServer.cpp.diff (2.3 KB) - added by kaoutsis 11 years ago.
src_apps_processcontroller_PCWorld.cpp.diff (2.0 KB) - added by kaoutsis 11 years ago.

Download all attachments as: .zip

Change History (41)

Changed 11 years ago by kaoutsis

Attachment: devfs.cpp.diff added

Changed 11 years ago by kaoutsis

Attachment: team.cpp.diff added

Changed 11 years ago by kaoutsis

Attachment: HWindow.cpp-v2.diff added

with style changes

Changed 11 years ago by kaoutsis

Attachment: HWindow.cpp.diff added

only with the find_directory changes

Changed 11 years ago by kaoutsis

Attachment: module.cpp.diff added

Changed 11 years ago by kaoutsis

comment:1 Changed 11 years ago by kaoutsis

Status: newassigned

comment:2 Changed 11 years ago by kaoutsis

Comments about the src_preferences_screensaver_ScreenSaverApp.cpp.diff: Actually there are modifications in two files: a) src/preferences/screensaver/ScreenSaverApp.cpp b) src/preferences/screensaver/ScreenSaverWindow.cpp Besides the find_directory fix, i take the freedom to add an enhancement: Now the ScreenSaver pref. can receive more than one ref at a time; so you can add one or more screensavers with the 'add' button or you can simply drag and drop the screensavers into the ScreenSaver window :-)

Changed 11 years ago by kaoutsis

comment:3 Changed 11 years ago by kaoutsis

Comments about the src_preferences_screensaver_ScreenSaverApp.cpp.diff: Τhe ScreenSaver pref. is now capable to distinguish the real screensavers and installs only them to the user addon directory, when the user drop files in the ScreenSaver pref. window; tested successfully with the collection of screensavers attached to #511

comment:4 Changed 11 years ago by korli

HWindow.cpp, TrackerInitialState.cpp and FontManager.cpp patched

comment:5 in reply to:  4 Changed 11 years ago by kaoutsis

Replying to korli:

HWindow.cpp, TrackerInitialState.cpp and FontManager.cpp patched

May be TrackerInitialState.cpp is forgotten?

Changed 11 years ago by kaoutsis

comment:6 Changed 11 years ago by kaoutsis

comments on src_bin_desklink_desklink.cpp.diff:

  • the usual find_directory() fix
  • i took the freedom, to add a 'About desklink...' menu item to the pop up menu.

But i used the asynchronous Go() version (with the NULL argument); The synchronous version ate all the cpu, but i don't know if that is a bug, since the alert was inside the MessageReceived() loop. The asynchronous Go() version seems to work fine, though.

comment:7 Changed 11 years ago by korli

Applied desklink.cpp.diff in hrev25462. I factorized the alert code and change a few things.

Changed 11 years ago by kaoutsis

comment:8 Changed 11 years ago by kaoutsis

src_system_kernel_main.cpp.diff: updated to hrev25481

comment:9 Changed 11 years ago by kaoutsis

src_preferences_screensaver_ScreenSaverApp.cpp.diff:

  • fixed a typo, that i introduced to the 'check for a screensaver' routine:

the buffer that is used to contain the informations in order to examine if the file is a screensaver was not use memset correctly (only the first 4096 bytes were filled with 'a', not the entire buffer); that caused a bug: in some rare cases, the buffer when is reused, may contains the string 'StartSaver', that was left there from the examination of a previous real screensaver file, since these addresses would be part of the program's free address space, resulting in a wrong decision about the file in question. The fix makes now the routine more efficient: it doesn't use memset to fill the whole buffer with 'a' every time that the buffer is initialized, but only at the point where it finds a screensaver file, it fills the specific positions of the string with 'a', thus memset is using only 10 bytes.

comment:10 Changed 11 years ago by stippi

Owner: changed from kaoutsis to stippi
Status: assignednew

I am looking into the remaining patches. They all look pretty good, I am just thinking that the Append() calls need error checking and for some of these, I consider using hardcoded paths as fallbacks.

comment:11 Changed 11 years ago by stippi

Status: newassigned

comment:12 Changed 11 years ago by axeld

I don't know if this is really necessary - if Append() already fails, there doesn't seem to be much memory left to do anything useful, anyway :-) Well, the stuff in the kernel might do so.

comment:13 Changed 11 years ago by stippi

devfs.cpp.diff applied in hrev25503.

comment:14 Changed 11 years ago by stippi

team.cpp.diff applied in hrev25506.

comment:15 Changed 11 years ago by stippi

system_kernel_main.cpp.diff applied in hrev25509.

comment:16 Changed 11 years ago by korli

About src_preferences_screensaver_ScreenSaverApp.cpp.diff, I'm not sure the way the 'check for a screensaver' is done is good. Why not let the screensaver loading do its job ?

comment:17 Changed 11 years ago by bonefish

I'm actually not quite sure, whether I find it a good idea to use find_directory() in system components like the kernel or the runtime loader. They just complicate things without real benefit. I mean, the whole point of using find_directory() in applications is to still work as expected when the system's directory layout changes. But these components *are* the system. I think I'd rather use a common single header file that defines the paths.

comment:18 Changed 11 years ago by axeld

That common single header sounds like a good idea. It should then be used by the kernel and find_directory() itself (if viable). find_directory() would only make sense in the kernel as well, if we make these paths configurable - which I don't think is really necessary.

comment:19 in reply to:  16 ; Changed 11 years ago by kaoutsis

Replying to korli:

About src_preferences_screensaver_ScreenSaverApp.cpp.diff, I'm not sure the way the 'check for a screensaver' is done is good. Why not let the screensaver loading do its job ?

I'm not sure, either :-)

There are two issues here: 1) The basic idea was that the screensaver list should contains only screensaver files.

Imagine the scenario: a user drops a readme file, and that readme appears in the screensaver list. If we can prevent that, i think that to check for a valid screensaver, is a good idea. Of course, if we want the screensaver list to contain only screensaver files, there is more work to be done. For example, the user may drop the file(s) directly to the addons directory, then the screensaver list should updated with only the valid screensaver files, and if the user insist to drop the file in the ScreenSaver window, he will get the alert info: file x, is not a screensaver file.

2) Now the 'routine' check: Obviously, the routine is some kind of practical algorithm :-) All the screensaver-binary-files that i could tested contained the 'StartSaver' string somewhere inside a 80k limit (actually the real limit is somewhere between 40k, i increased to 80k just in case). Of course, a valid screensaver should use the StartSaver() function (i hope this hypothesis is correct). Giving the fact, that the user hopefully, will drop screensaver files, the routine checks first the first 4096 bytes, then 4096 * 2, then 4096 * 3, and so on; it usually finds the screensaver after 4 or 5 tries. Now if the user drops a irrelevant file then indeed the check hits the limits (n = 20 times and each time checks 4096 * 1, 4096 * 2, ...,4096 * n bytes).

If you think that 1. is a good idea, then we can think over 2. It's up to you :-)

comment:20 in reply to:  19 ; Changed 11 years ago by korli

Replying to kaoutsis:

If you think that 1. is a good idea, then we can think over 2.

I'm OK for 1. I think 2. is not the way to go. I would prefer the actual code to load the addon:

image = load_add_on(path);
if (image != B_OK)
   ...
if (get_image_symbol((image, "instantiate_screen_saver", B_SYMBOL_TYPE_TEXT, (void **)&instantiate) != B_OK)
   ...
unload_add_on(image);

Isn't it better?

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

comment:21 in reply to:  20 Changed 11 years ago by kaoutsis

Replying to korli:

Replying to kaoutsis:

If you think that 1. is a good idea, then we can think over 2.

I'm OK for 1. I think 2. is not the way to go. I would prefer the actual code to load the addon: Isn't it better?

I believe yes. I will try an implementation based upon this idea; i will take my time, and i will come back later with an updated patch, OK?

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

comment:22 Changed 11 years ago by kaoutsis

src_preferences_screensaver_ScreenSaverApp.cpp.2.diff:

There are a number of changes in order to make the ScreenSaver pref. contains only screensavers in the list, notably:

1) The use of load_add_on() and get_image_symbol() functions.

Note however that there is a difference in relation with the previous implementation: Besides the really irrelevant files, the routine now 'cuts off' also screensaver files that can't loaded correctly, there are two cases: a) screesaver-files with missing library, e.g libGLU.so b) screensaver-files with unresolved symbols, e.g elf_resolve_symbol: could not resolve symbol 'ReadFrames11BMediaTrackPcPxP12media_header'

2) The use of watch_node() in order to monitor the user's Screen Savers addons directory. Since we are moving to a multiuser environment, i think there isn't need to monitor other dirs, the user would access these dirs read-only.

  • Although the list is updated with only the valid screensaver files

on the fly, namely when the ScreenSaver pref. is running and the user drops the file(s) directly to the addons directory, the ScrenSaver pref. crashes is some cases if the user selects some item from the list; i still don't know why and where.

comment:23 Changed 11 years ago by kaoutsis

src_servers_registrar_TRoster.cpp.diff:

  • removed unused variable

static const char *const kVitalSystemAppPathPrefix

  • use of find_directory() & BPath & co.
  • use of D(PRINT()) macro to return errors

Changed 11 years ago by kaoutsis

comment:24 Changed 11 years ago by kaoutsis

src_servers_debug_DebugServer.cpp.diff:

  • updated to hrev25560
  • also take into account the returned status values

of BPath::Append()

Changed 11 years ago by kaoutsis

Changed 11 years ago by kaoutsis

comment:25 Changed 11 years ago by kaoutsis

src_apps_processcontroller_PCWorld.cpp.diff:

  • find_directory() and friends.

comment:26 Changed 10 years ago by leavengood

Cc: leavengood@… added

I will add the ScreenSaver diff, this looks pretty useful and I am already working on the ScreenSaver preferences anyhow.

comment:27 Changed 8 years ago by scottmc

Have all of these patches been applied yet? Are there more to come?

Note: See TracTickets for help on using tickets.