Opened 9 years ago

Closed 9 years ago

#12105 closed bug (fixed)

[Tracker][Add-On API] process_refs gets wrong directory entry_ref

Reported by: ahwayakchih Owned by: jscipione
Priority: blocker Milestone: R1/beta1
Component: Applications/Tracker Version: R1/Development
Keywords: process_refs Cc:
Blocked By: Blocking: #12027
Platform: All

Description

Hi,

I just tried nightly (after a looong time) inside virtualbox to try to rebuild my tiny apps on Haiku and noticed different behaviour than the one described in BeBook (https://www.haiku-os.org/legacy-docs/bebook/TheTracker_AddOnProtocol.html#process_refs).

I may be doing something wrong, but it looks like the Tracker passes wrong entry_ref to the add-on application. Instead of passing entry_ref to the directory, i'm getting entry_ref to the selected file.

I managed to workaround that by using BEntry.IsFile() and getting parent directory before continuing, but that was not needed before.

Was that intentionally changed without changing header files (first parameter to process_refs is called directory in header files)?

Regards, ahwayakchih

Attachments (1)

test.zip (3.9 KB ) - added by ahwayakchih 9 years ago.
Test Add-On

Download all attachments as: .zip

Change History (10)

by ahwayakchih, 9 years ago

Attachment: test.zip added

Test Add-On

comment:1 by ahwayakchih, 9 years ago

Test shows that Tracker passes entry_ref to one of the selected entries, instead of the directory they are located in.

comment:2 by pulkomandy, 9 years ago

Owner: changed from axeld to waddlesplash
Status: newassigned

comment:3 by ahwayakchih, 9 years ago

I think the bug is in here:

https://github.com/haiku/haiku/blob/master/src/kits/tracker/ContainerWindow.cpp#L3228

For easier reading, section of code (lines 3226-3232):

    const entry_ref* modelRef = TargetModel()->IsContainer()
            && selectionList->ItemAt(0) != NULL
        ? selectionList->ItemAt(0)->TargetModel()->EntryRef()
        : TargetModel()->EntryRef();

    LaunchInNewThread("Add-on", B_NORMAL_PRIORITY, &AddOnThread, refs,
        addonRef, *modelRef);

On line 3228, it gets entry_ref of the first item, instead of entry_ref of first item's directory.

I may be wrong, but i think that whole ternary condition is incorrect. shouldn't it be more like:

    const entry_ref* modelRef = !TargetModel()->IsContainer()
            && selectionList->ItemAt(0) == NULL
        ? selectionList->ItemAt(0)->TargetModel()->EntryRef()
        : TargetModel()->EntryRef();

    LaunchInNewThread("Add-on", B_NORMAL_PRIORITY, &AddOnThread, refs,
        addonRef, *modelRef);

(notice the first condition - added missing "!" there :). That would be much better, but still not ideal: don't know if that may happen in practice, but if TargetModel was not container and there would be no item selected, we would still get entry_ref to that weird TargetModel, in which case it probably would be better to ignore whole action and return earlier (and/or show some error message?).

Version 1, edited 9 years ago by ahwayakchih (previous) (next) (diff)

comment:4 by ahwayakchih, 9 years ago

After looking into it some more, now i know why the condition was written like that: https://github.com/haiku/haiku/commit/2634c39731fdbbbd5324944bffb5075abe71619f

So, if we do not want to pass the query / virtual directory as ref, only first of their items, then we should get first item's directory.

What other TargetModel there can be? If it is not a directory, virtual directory nor a query?

Maybe it should just check if it is directory (in which case, get TargetModel entry ref) or not (in which case get entry_ref of first items parent directory)?

Something like this:

    const entry_ref* modelRef;
    if (TargetModel()->IsDirectory())
    {
        modelRef = TargetModel()->EntryRef();
    }
    else if (selectionList->ItemAt(0) != NULL)
    {
        // This is just a pseudo code for example purposes.
        // There is no ParentEntryRef function AFAIK :).
        modelRef = selectionList->ItemAt(0)->TargetModel()->ParentEntryRef();
    }
    else
    {
        // Not a directory, and no items selected?
        // Show some error
        // Cleanup
        delete refs; // refs is created above.
                     // we could move condition that to prevent object creation instead
        return;
    }

    LaunchInNewThread("Add-on", B_NORMAL_PRIORITY, &AddOnThread, refs,
        addonRef, *modelRef);

comment:5 by korli, 9 years ago

Owner: changed from waddlesplash to jscipione

It should be a regression resulting from this commit

comment:6 by axeld, 9 years ago

Milestone: UnscheduledR1/beta1

Looks like OpenTargetFolder was using the directoryRef incorrectly, and jscipione then fixed it in the wrong way. This might also explain why archiving using the Tracker add-on suddenly didn't work anymore.

comment:8 by waddlesplash, 9 years ago

Priority: normalblocker

Bumping to 'blocker' priority, as this breaks nearly all Tracker addons.

comment:9 by waddlesplash, 9 years ago

Blocking: 12027 added

comment:10 by jscipione, 9 years ago

Resolution: fixed
Status: assignedclosed

Fixed in hrev49314

Note: See TracTickets for help on using tickets.