[haiku-commits] Re: haiku: hrev50835 - src/preferences/repositories data/artwork/icons

  • From: Michael Lotz <mmlr@xxxxxxxx>
  • To: haiku-commits@xxxxxxxxxxxxx
  • Date: Sun, 8 Jan 2017 15:13:12 +0100

Hi

On 07.01.2017 19:51, waddlesplash@xxxxxxxxx wrote:

hrev50835 adds 1 changeset to branch 'master'
overview: 
http://cgit.haiku-os.org/haiku/log/?qt=range&q=5bf2b6eb74cb+%5E564aac42098c

5bf2b6eb74cb: Add the new Repositories preflet.

  Also includes changes to HaikuDepot to change wording and add the menu item
  to open the Repositories preflet.

  Signed-off-by: Augustin Cavalier <waddlesplash@xxxxxxxxx>
  Closes #13147.

                                     [ Brian Hill <supernova@xxxxxxxxxxxx> ]

Always nice to see new contributors!

While glancing over the diff some things caught my eye (mostly coding style):

+++ b/src/preferences/repositories/AddRepoWindow.cpp
diff --git a/src/preferences/repositories/AddRepoWindow.h 
b/src/preferences/repositories/AddRepoWindow.h
new file mode 100644
index 0000000..c6c1758
--- /dev/null
+++ b/src/preferences/repositories/AddRepoWindow.h
@@ -0,0 +1,35 @@
+
+private:
+       BTextControl*                   fText;
+       BButton*                                fAddButton;
+       BButton*                                fCancelButton;
+       BMessenger                              fReplyMessenger;
+       status_t                                _GetClipboardData();
+};

We usually sort the methods first and the members last within their visibility, separated by an empty line.

diff --git a/src/preferences/repositories/RepositoriesSettings.cpp 
b/src/preferences/repositories/RepositoriesSettings.cpp
new file mode 100644
index 0000000..02629ee
--- /dev/null
+++ b/src/preferences/repositories/RepositoriesSettings.cpp
>
> ...
>
+RepositoriesSettings::RepositoriesSettings()
+{
>
> ...
>
+               // Create default repos
+               BStringList nameList, urlList;
+               int32 count = (sizeof(kDefaultRepos) / sizeof(Repository));
+               for (int16 index = 0; index < count; index++) {
+                       nameList.Add(kDefaultRepos[index].name);
+                       urlList.Add(kDefaultRepos[index].url);

Why are there hardcoded default repositories? I see there's also code to enumerate the actually configured repositories from the package kit, so what are these used for? And why duplicate the repo list in another settings file and not always use the package kit to enumerate the installed repositories?

diff --git a/src/preferences/repositories/RepositoriesView.cpp 
b/src/preferences/repositories/RepositoriesView.cpp
new file mode 100644
index 0000000..27f7070
--- /dev/null
+++ b/src/preferences/repositories/RepositoriesView.cpp
>
> ...
>
+
+               case STATUS_VIEW_COMPLETED_TIMEOUT: {
>
> ...
>
+               }
+               default:
+                       BView::MessageReceived(message);

The coding guidelines suggest to put the opening brace in case blocks on a new line (same as for functions/methods) and have an empty line between the case blocks.

+       if (kNewRepoDefaultName.Compare(rowItem->Name()) == 0
+               && newName.Compare("") != 0)
+               rowItem->SetName(newName.String());

Ifs (and others) always need braces around their bodies if either the condition or the body is more than a single line. Here it's the latter, but there's at least one other place in the diff where it is the former (in TaskTimer::Start).

+BString
+RepositoriesView::_GetRootUrl(BString url)
+{
+       // Find the protocol if it exists
+       int32 ww = url.FindFirst("://");
+       if (ww == B_ERROR)
+               ww = 0;
+       else
+               ww += 3;
+       // Find second /
+       int32 rootEnd = url.FindFirst("/", ww + 1);
+       if (rootEnd == B_ERROR)
+               return url;
+       rootEnd = url.FindFirst("/", rootEnd + 1);
+       if (rootEnd == B_ERROR)
+               return url;
+       else
+               return url.Truncate(rootEnd);
+}

Please use the BUrl class instead of manual URL parsing (here and also everywhere else). Manual parsing like this is at least unhandy and may very well miss special cases (what about basic auth credentials for example).

+status_t
+RepositoriesView::_EmptyList()
+{
+       BRow* row;
+       while ((row = fListView->RowAt((int32)0, NULL)) != NULL) {
+               fListView->RemoveRow(row);
+               delete row;
+       }
+       return B_OK;
+}

This would be slightly more efficient by using:

while ((row = fListView->RemoveItem((int32)0))
        delete row;

Although the coding guidelines discourage assignments inside loop conditions like this...

+RepoRow*
+RepositoriesView::_AddRepo(BString name, BString url, bool enabled)
+{
>
> ...
>
+       // Find if the repo already exists in list
+       for (index = 0; index < listCount; index++) {
+               RepoRow* repoItem = 
dynamic_cast<RepoRow*>(fListView->RowAt(index));
+               if (url.ICompare(repoItem->Url()) == 0) {
+                       // update name and enabled values
+                       if (name.Compare(repoItem->Name()) != 0)
+                               repoItem->SetName(name.String());
+                       repoItem->SetEnabled(enabled);
+                       addedRow = repoItem;
+               }
+       }
+       if (addedRow == NULL) {
+               addedRow = new RepoRow(name, url, enabled);
+               fListView->AddRow(addedRow);
+       }
+       return addedRow;
+}

An early return in the loop would ensure that the rest of the list isn't needlessly iterated over further and remove the need for the if (addedRow == NULL) check.

+void
+RepositoriesView::_UpdateButtons()
+{
+       RepoRow* rowItem = 
dynamic_cast<RepoRow*>(fListView->CurrentSelection());
+       // At least one row is selected
+       if (rowItem) {
+               bool someAreEnabled = false,
+                       someAreDisabled = false,
+                       someAreInQueue = false;

The coding guidelines discourage multiple variable declarations like this. They should be individual declarations, one on each line.

Otherwise this seems like a nice addition.

Regards,
Michael

Other related posts:

  1. Home
  2. haiku-commits
  3. Archive
  4. 01-2017