Opened 8 years ago

Closed 6 years ago

#7951 closed enhancement (fixed)

[LaunchBox] add Open containing folder option (easy)

Reported by: diver Owned by: stippi
Priority: normal Milestone: R1
Component: Applications/LaunchBox Version: R1/Development
Keywords: gsoc2012 Cc:
Blocked By: Blocking:
Has a Patch: yes Platform: All

Description

It would be handy to be able to open containing folder if some problem arise or just quickly go to app folder. We already have "Open Target Folder" tracker add-on, so it might be better to add Tracker's Add-ons menu instead.

Attachments (3)

new2.patch (3.5 KB) - added by abhiin1947 7 years ago.
By: abhiin1947
new3.patch (3.5 KB) - added by abhiin1947 7 years ago.
By: abhiin1947
new5.patch (3.5 KB) - added by abhiin1947 7 years ago.
patch following the coding guidelines

Download all attachments as: .zip

Change History (15)

comment:1 Changed 8 years ago by stippi

Summary: [LaunchBox] add Open target folder option[LaunchBox] add Open containing folder option (easy)

comment:2 Changed 8 years ago by eml

Sorry, could you explain this one a bit further? Found this one in the easy tasks lists but I don't fully understand it.

comment:3 Changed 8 years ago by humdinger

The LaunchBox contains links to objects. It would be nice to be able to right-click such an object and have a menu to open the folder it's located in. Tracker has such a function under "Add-ons | Open target folder (ALT+OPT+O)" when you right-click a symlinked object -- see all entries of "Deskbar | Applications" for example.

Instead of a single new menu item in LaunchBox to "Open target folder", it would be even nicer to provide the same "Add-Ons" menu Tracker is showing. It's contents depends on the filetype of the object and there might be other interesting Tracker Add-Ons for the object.

Changed 7 years ago by abhiin1947

Attachment: new2.patch added

By: abhiin1947

comment:4 Changed 7 years ago by abhiin1947

Has a Patch: set

Changed 7 years ago by abhiin1947

Attachment: new3.patch added

By: abhiin1947

comment:5 Changed 7 years ago by abhiin1947

I've made mistake in new2.patch...the new3.patch file is working...try it out and give me your feedback

comment:6 Changed 7 years ago by axeld

Thanks for the patch! It looks basically good, and everything seems to be at the right place. I have a few suggestions, though, and there are a few coding style violations:

  • Please don't use abbreviations that hide the meaning of the word. For example, the CONT in the message constant should just stay CONTAINING.
  • Why add the button as reference if you only ever use its Ref()? You could add the entry_ref directly to the message instead.
  • If the method you are calling isn't documented to return a value different from B_OK on success, you should only test ==, not >=.
  • We prefer to use button->Ref() != NULL in comparisons to make it a boolean comparison for clarity.
  • The code you are using does not do any error checking. The only effect this will have in this case is that nothing happens, so that's probably acceptable. In any case, you might want to only add that menu item if the button is not empty, and has a valid entry_ref in the first place.
  • Instead of nesting to if's without any extra code, you could use a && and save an extra indenting level.
  • The variable names you chose aren't coding style compliant; we use camel case, and variable names always start with a lower case letter. In other words, Opentarget would become openTarget, Openmsger could be called tracker instead, or, if you prefer trackerMessenger, etc.
  • You sometimes use no spacing, like after the second if, or around the = operator.

Would be great if you could take another look at your code with the comments in mind :-)

comment:7 Changed 7 years ago by abhiin1947

Thank you for your suggestions..I have made a new patch rectifying most of them

  1. There are a lot of cases where the if conditions don't have an RHS..is this a part of the guideline?
  1. I have used the button as a reference because I thought it might help while creating the add-on menu

If I have made any more mistakes I'll be happy to rectify them..Appreciate your reply

Changed 7 years ago by abhiin1947

Attachment: new5.patch added

patch following the coding guidelines

comment:8 in reply to:  7 Changed 7 years ago by axeld

Replying to abhiin1947:

  1. There are a lot of cases where the if conditions don't have an RHS..is this a part of the guideline?

RHS = right hand side? The condition should always be a boolean condition - if it already is, there is no need for a == true, for example.

Also, our coding style is evolving over time, too, and not everyone is always writing perfect code, anyway. We just try hard to follow it as much as possible, and try to only accept new code which follows it.

  1. I have used the button as a reference because I thought it might help while creating the add-on menu

What add-on menu do you mean?

Anyway, thanks for the update! Your patch looks very good now, and I would apply it as soon as possible; while I don't have much time right now, it'll happen next weekend at the latest.

comment:9 Changed 7 years ago by axeld

BTW is part of an upcoming GSoC application?

comment:10 Changed 7 years ago by diver

Wouldn't it be more convenient to reuse Add-Ons menu from Tracker?

comment:11 Changed 7 years ago by abhiin1947

Keywords: gsoc2012 added

comment:12 Changed 6 years ago by mmadia

Resolution: fixed
Status: newclosed

Applied in hrev45291. Thanks for the patch abhiin1947!

Note: See TracTickets for help on using tickets.