Opened 12 years ago

Last modified 3 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 12 years ago.
team.cpp.diff (1.4 KB ) - added by kaoutsis 12 years ago.
HWindow.cpp-v2.diff (15.2 KB ) - added by kaoutsis 12 years ago.
with style changes
HWindow.cpp.diff (1.4 KB ) - added by kaoutsis 12 years ago.
only with the find_directory changes
module.cpp.diff (1.2 KB ) - added by kaoutsis 12 years ago.
TrackerInitialState.cpp.diff (1.1 KB ) - added by kaoutsis 12 years ago.
src_servers_app_FontManager.cpp.diff (1.2 KB ) - added by kaoutsis 12 years ago.
src_bin_desklink_desklink.cpp.diff (3.8 KB ) - added by kaoutsis 12 years ago.
src_system_kernel_main.cpp.diff (19.3 KB ) - added by kaoutsis 12 years ago.
src_preferences_screensaver_ScreenSaverApp.cpp.diff (7.4 KB ) - added by kaoutsis 12 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)

by kaoutsis, 12 years ago

Attachment: devfs.cpp.diff added

by kaoutsis, 12 years ago

Attachment: team.cpp.diff added

by kaoutsis, 12 years ago

Attachment: HWindow.cpp-v2.diff added

with style changes

by kaoutsis, 12 years ago

Attachment: HWindow.cpp.diff added

only with the find_directory changes

by kaoutsis, 12 years ago

Attachment: module.cpp.diff added

by kaoutsis, 12 years ago

comment:1 by kaoutsis, 12 years ago

Status: newassigned

comment:2 by kaoutsis, 12 years ago

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 :-)

by kaoutsis, 12 years ago

comment:3 by kaoutsis, 12 years ago

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 by korli, 12 years ago

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

in reply to:  4 comment:5 by kaoutsis, 12 years ago

Replying to korli:

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

May be TrackerInitialState.cpp is forgotten?

by kaoutsis, 12 years ago

comment:6 by kaoutsis, 12 years ago

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 by korli, 12 years ago

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

by kaoutsis, 12 years ago

comment:8 by kaoutsis, 12 years ago

src_system_kernel_main.cpp.diff: updated to hrev25481

comment:9 by kaoutsis, 12 years ago

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 by stippi, 12 years ago

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 by stippi, 12 years ago

Status: newassigned

comment:12 by axeld, 12 years ago

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 by stippi, 12 years ago

devfs.cpp.diff applied in hrev25503.

comment:14 by stippi, 12 years ago

team.cpp.diff applied in hrev25506.

comment:15 by stippi, 12 years ago

system_kernel_main.cpp.diff applied in hrev25509.

comment:16 by korli, 12 years ago

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 by bonefish, 12 years ago

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 by axeld, 12 years ago

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.

in reply to:  16 ; comment:19 by kaoutsis, 12 years ago

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 :-)

in reply to:  19 ; comment:20 by korli, 12 years ago

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 3 years ago by pulkomandy (previous) (diff)

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

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 3 years ago by pulkomandy (previous) (diff)

comment:22 by kaoutsis, 11 years ago

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 by kaoutsis, 11 years ago

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

comment:24 by kaoutsis, 11 years ago

src_servers_debug_DebugServer.cpp.diff:

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

of BPath::Append()

comment:25 by kaoutsis, 11 years ago

src_apps_processcontroller_PCWorld.cpp.diff:

  • find_directory() and friends.

comment:26 by leavengood, 11 years ago

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 by scottmc, 9 years ago

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

Note: See TracTickets for help on using tickets.