Skip to content
This repository has been archived by the owner on May 1, 2023. It is now read-only.

Added a way to provide the database files to the DatabasePeerManager module. #91

Merged
merged 1 commit into from Mar 13, 2015

Conversation

jferlisi
Copy link
Contributor

Created an interface to provide database files at the time of a peer connection. This allows applications that create databases in directories other than the default one to have their data viewable in the Database section.

this(context, new DatabaseFilesProvider() {
@Override public Set<File> getDatabaseFiles() {
Set<File> databaseFiles = new HashSet<File>();
for (String filename : context.databaseList())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

touché, I didn't know about this API :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, so I did. Well that's weird, I have no memory of this at all. facepalm

@jasta
Copy link
Contributor

jasta commented Mar 12, 2015

I'm curious for your thoughts at a high level on the API for configuring modules. I believe at present you are writing something like:

new InspectorModulesProvider() {
  public Iterable<> get() {
    HashSet<> modules = new HashSet<>();
    Collections.add(modules, Stetho.defaultInspectorModulesProvider(context).get());
    replaceByType(Database.class, new Database(context, new MyDatabaseProvider(...)));
    return modules;
  }
}

We're doing this internally in FB4A/Messenger, but I'm really unhappy with it. Seems like these use cases should be expressed more conveniently, perhaps with some kind of pattern like:

Stetho.newInspectorModulesBuilder(context)
   .addDefaults()
   .add(new Database(...));
   .build();

Internally addDefaults would be a boolean which operated like:

for (ChromeDevtoolsDomain domain : mCustomModules) {
  modules.add(domain);
  customModuleNames.add(InspectorUtil.getDomainName(domain));
}
if (mAddDefaults) {
  if (!customModuleNames.contains("Database")) {
    modules.add(...);
  }
  ...
}

@jferlisi
Copy link
Contributor Author

I'm curious for your thoughts at a high level on the API for configuring modules.

I'm still playing around with things so I haven't given it a lot of thought. Right now what I have done is copied the list of modules from the defaults and add each of them individually.

ArrayList<ChromeDevtoolsDomain> modules = new ArrayList<ChromeDevtoolsDomain>();
modules.add(new Console());
modules.add(new CSS());
...
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.HONEYCOMB) {
  modules.add(new Database(context, myDatabaseFilesProvider));
}
return modules;

I do like your proposed method though.

* @deprecated use
*/
@Deprecated
public DatabasePeerManager(final Context context) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: don't need final

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Certainly not anymore. I'll remove it.

@jasta
Copy link
Contributor

jasta commented Mar 13, 2015

Looks good, clean up the minor nits then rebase/squash and we can merge.

@jferlisi
Copy link
Contributor Author

Do you happen to have an Intellij code style file published anywhere?

@jasta
Copy link
Contributor

jasta commented Mar 13, 2015

@jferlisi no, working on it tho :)

@jferlisi
Copy link
Contributor Author

@jasta Pushed changes.

@jasta
Copy link
Contributor

jasta commented Mar 13, 2015

Looks good, please squash into 1 commit (http://gitready.com/advanced/2009/02/10/squashing-commits-with-rebase.html), then git push -f into your github fork branch.

@jferlisi
Copy link
Contributor Author

@jasta Done, thanks for the help on this.

jasta added a commit that referenced this pull request Mar 13, 2015
Added a way to provide the database files to the DatabasePeerManager module.
@jasta jasta merged commit 32f6c63 into facebookarchive:master Mar 13, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants