changes in Logger

pass around Logger rather than Logger::Callback,
use aggregate initialization when passing function to a Logger arg

include/gemmi/assembly.hpp

@@ -88,7 +88,7 @@ inline void ensure_unique_chain_name(const Model& model, Chain& chain) {
 }
 
 GEMMI_DLL Model make_assembly(const Assembly& assembly, const Model& model,
-                              HowToNameCopiedChain how, const Logger::Callback& logging);
+                              HowToNameCopiedChain how, const Logger& logging);
 
 inline Assembly pseudo_assembly_for_unit_cell(const UnitCell& cell) {
   Assembly assembly("unit_cell");
@@ -105,7 +105,7 @@ inline Assembly pseudo_assembly_for_unit_cell(const UnitCell& cell) {
 /// If called with assembly_name="unit_cell" changes structure to unit cell (P1).
 /// \par keep_spacegroup preserves space group and unit cell - is it needed?
 GEMMI_DLL void transform_to_assembly(Structure& st, const std::string& assembly_name,
-                                     HowToNameCopiedChain how, const Logger::Callback& logging,
+                                     HowToNameCopiedChain how, const Logger& logging,
                                      bool keep_spacegroup=false, double merge_dist=0.2);
 
 

include/gemmi/logger.hpp

@@ -12,38 +12,57 @@
 
 namespace gemmi {
 
-/// Passes notes and warnings to a callback function.
-/// Note: if the callback is not set, notes are ignored but warnings
-/// are thrown as exceptions.
+/// Passes messages (including warnings/errors) to a callback function.
+/// Messages are passed as strings without a newline character.
+/// Messages have severity levels similar syslog:
+///  7=debug, 6=info (all but debug), 5=notice, 3=error
+/// A numeric threshold can be set to limit the messages (see below).
+/// Quirk: if a callback is not set, errors are thrown as exceptions.
 struct Logger {
-  using Callback = std::function<void(const std::string&)>;
-  Callback callback;
+  /// A function that handles messages.
+  std::function<void(const std::string&)> callback;
+  /// Pass messages of this level and all lower (more severe) levels:
+  /// 7=all messages, 6=all but debug, 0=none
+  int threshold = 6;
 
-  // For internal use in functions that produce messages: suspending when
-  // the same function is called multiple times avoids duplicated messages.
-  bool suspended = false;
+  /// suspend() and resume() are used internally to avoid duplicate messages
+  /// when the same function is called (internally) multiple times.
+  void suspend() { threshold -= 100; }
+  void resume()  { threshold += 100; }
 
-  // Send warning.
+  /// Send a debug message.
+  template<class... Args> void debug(Args const&... args) const {
+    if (threshold >= 7 && callback)
+      callback(cat("Debug: ", args...));
+  }
+
+  /// Send a message without any prefix.
+  template<class... Args> void mesg(Args const&... args) const {
+    if (threshold >= 6 && callback)
+      callback(cat(args...));
+  }
+
+  /// Send a note (a notice, a significant message).
+  template<class... Args> void note(Args const&... args) const {
+    if (threshold >= 5 && callback)
+      callback(cat("Note: ", args...));
+  }
+
+  /// Send a warning/error. Unrecoverable errors are thrown directly and
+  /// don't go through this class, so here we're left with errors that
+  /// can be downgraded to warnings. If a callback is set, the message is
+  /// passed as a warning; otherwise it's thrown as a std::runtime_error.
+  /// (Admittedly, it's a questionable design.)
   template<class... Args> GEMMI_COLD void err(Args const&... args) const {
-    if (!suspended) {
+    if (threshold >= 3) {
       std::string msg = cat(args...);
       if (callback == nullptr)
         fail(msg);
       callback("Warning: " + msg);
     }
   }
 
-  // Send note.
-  template<class... Args> void note(Args const&... args) const {
-    if (!suspended && callback)
-      callback(cat("Note: ", args...));
-  }
-
-  // Send a message without any prefix
-  template<class... Args> void mesg(Args const&... args) const {
-    if (!suspended && callback)
-      callback(cat(args...));
-  }
+  // predefined callbacks
 
   /// to be used as: logger.callback = Logger::to_stderr;
   static void to_stderr(const std::string& s) {

include/gemmi/monlib.hpp

@@ -232,10 +232,10 @@ struct GEMMI_DLL MonLib {
   /// Returns true if all requested monomers were added.
   bool read_monomer_lib(const std::string& monomer_dir_,
                         const std::vector<std::string>& resnames,
-                        const Logger::Callback& logging);
+                        const Logger& logger);
 
   double find_ideal_distance(const const_CRA& cra1, const const_CRA& cra2) const;
-  void update_old_atom_names(Structure& st, const Logger::Callback& logging) const;
+  void update_old_atom_names(Structure& st, const Logger& logger) const;
 };
 
 // to be deprecated
@@ -247,10 +247,10 @@ inline MonLib read_monomer_lib(const std::string& monomer_dir,
   if (!libin.empty())
     monlib.read_monomer_cif(libin);
   std::string error;
-  Logger::Callback logging;
+  Logger logger;
   if (!ignore_missing)
-    logging = [&error](const std::string& s) { cat_to(error, s, '\n'); };
-  bool ok = monlib.read_monomer_lib(monomer_dir, resnames, logging);
+    logger.callback = [&error](const std::string& s) { cat_to(error, s, '\n'); };
+  bool ok = monlib.read_monomer_lib(monomer_dir, resnames, logger);
   if (!ignore_missing && !ok) {
     error += "Please create definitions for missing monomers.";
     fail(error);

include/gemmi/topo.hpp

@@ -289,7 +289,7 @@ struct GEMMI_DLL Topo {
 GEMMI_DLL std::unique_ptr<Topo>
 prepare_topology(Structure& st, MonLib& monlib, size_t model_index,
                  HydrogenChange h_change, bool reorder,
-                 const Logger::Callback& callback={}, bool ignore_unknown_links=false,
+                 const Logger& logger={}, bool ignore_unknown_links=false,
                  bool use_cispeps=false);
 
 

prog/convert.cpp

@@ -421,9 +421,8 @@ void convert(gemmi::Structure& st,
   if (output_type == CoorFormat::Pdb)
     how = HowToNameCopiedChain::Short;
   if (options[AsAssembly]) {
-    gemmi::Logger::Callback callback = options[Verbose] ? &gemmi::Logger::to_stderr
-                                                        : &gemmi::Logger::nop;
-    gemmi::transform_to_assembly(st, options[AsAssembly].arg, how, callback);
+    auto callback = options[Verbose] ? &gemmi::Logger::to_stderr : &gemmi::Logger::nop;
+    gemmi::transform_to_assembly(st, options[AsAssembly].arg, how, {callback});
   }
 
   if (options[ExpandNcs]) {

prog/crd.cpp

@@ -122,7 +122,7 @@ int GEMMI_MAIN(int argc, char **argv) {
     else
       h_change = HydrogenChange::ReAddButWater;
     auto topo = prepare_topology(st, monlib, 0, h_change, reorder,
-                                 &Logger::to_stderr, ignore_unknown_links, use_cispeps);
+                                 {&Logger::to_stderr}, ignore_unknown_links, use_cispeps);
     if (!use_cispeps)
       topo->set_cispeps_in_structure(st);
     if (verbose)

prog/h.cpp

@@ -127,10 +127,10 @@ int GEMMI_MAIN(int argc, char **argv) {
       if (!wanted.empty())
         gemmi::fail("Please create definitions for missing monomers.");
       if (p.options[Update])
-        monlib.update_old_atom_names(st, gemmi::Logger::to_stdout);
+        monlib.update_old_atom_names(st, {&gemmi::Logger::to_stdout});
       for (size_t i = 0; i != st.models.size(); ++i) {
         // preparing topology modifies hydrogens in the model
-        prepare_topology(st, monlib, i, h_change, p.options[Sort], &gemmi::Logger::to_stderr);
+        prepare_topology(st, monlib, i, h_change, p.options[Sort], {&gemmi::Logger::to_stderr});
       }
     }
     if (p.options[Verbose])

prog/monlib_opt.cpp

@@ -70,7 +70,7 @@ void read_monomer_lib_and_user_files(gemmi::MonLib& monlib,
   }
   if (args.verbose)
     fprintf(stderr, "Reading monomer library...\n");
-  monlib.read_monomer_lib(args.monomer_dir, wanted, gemmi::Logger::to_stderr);
+  monlib.read_monomer_lib(args.monomer_dir, wanted, {&gemmi::Logger::to_stderr});
   auto is_found = [&](const std::string& s) { return monlib.monomers.count(s); };
   gemmi::vector_remove_if(wanted, is_found);
   if (libin) {

python/common.h

@@ -9,7 +9,7 @@
 #endif
 
 #include <nanobind/nanobind.h>  // IWYU pragma: export
-#include <functional>  // for function
+#include <gemmi/logger.hpp>     // for Logger
 
 #if defined(__clang__)
   #pragma clang diagnostic pop
@@ -119,20 +119,18 @@ inline nb::object handle_numpy_array_args(const nb::object& o, nb::handle dtype,
 }
 
 namespace nanobind { namespace detail {
-// for Logger::Callback, without depending on logger.hpp
-using LoggerCallback = std::function<void(const std::string&)>;
-template <> struct type_caster<LoggerCallback> {
-  NB_TYPE_CASTER(LoggerCallback, const_name("object"))
+template <> struct type_caster<gemmi::Logger> {
+  NB_TYPE_CASTER(gemmi::Logger, const_name("object"))
   bool from_python(handle src, uint8_t, cleanup_list *) noexcept {
     if (src.is_none())
-      value = nullptr;
+      value = {nullptr};
     else if (nb::hasattr(src, "write") && nb::hasattr(src, "flush"))
-      value = [obj=nb::borrow(src)](const std::string& s) {
+      value = {[obj=nb::borrow(src)](const std::string& s) {
         obj.attr("write")(s + "\n");
         obj.attr("flush")();
-      };
+      }};
     else if (PyCallable_Check(src.ptr()))
-      value = [obj=nb::borrow(src)](const std::string& s) { obj(s); };
+      value = {[obj=nb::borrow(src)](const std::string& s) { obj(s); }};
     else
       return false;
     return true;

python/mtz.cpp

@@ -111,8 +111,8 @@ void add_mtz(nb::module_& m) {
     .def_rw("batches", &Mtz::batches)
     .def_rw("history", &Mtz::history)
     .def_rw("appended_text", &Mtz::appended_text)
-    .def("set_logging", [](Mtz& self, Logger::Callback&& logging) {
-        self.logger.callback = std::move(logging);
+    .def("set_logging", [](Mtz& self, Logger&& logger) {
+        self.logger = std::move(logger);
     }, nb::arg().none())
     .def("resolution_high", &Mtz::resolution_high)
     .def("resolution_low", &Mtz::resolution_low)
@@ -346,9 +346,9 @@ void add_mtz(nb::module_& m) {
     .def("clone", [](const Mtz::Batch& self) { return new Mtz::Batch(self); })
     ;
 
-  m.def("read_mtz_file", [](const std::string& path, Logger::Callback&& logging) {
+  m.def("read_mtz_file", [](const std::string& path, Logger&& logging) {
     std::unique_ptr<Mtz> mtz(new Mtz);
-    mtz->logger.callback = std::move(logging);
+    mtz->logger = std::move(logging);
     mtz->read_file_gz(path, true);
     return mtz.release();
   }, nb::arg("path"), nb::arg("logging")=nb::none());

src/assembly.cpp

@@ -286,12 +286,12 @@ void finalize_expansion(Structure& st, const AssemblyMapping& mapping,
 } // anonymous namespace
 
 Model make_assembly(const Assembly& assembly, const Model& model,
-                    HowToNameCopiedChain how, const Logger::Callback& logging) {
-  return make_assembly_(assembly, model, how, Logger{logging}, nullptr);
+                    HowToNameCopiedChain how, const Logger& logging) {
+  return make_assembly_(assembly, model, how, logging, nullptr);
 }
 
 void transform_to_assembly(Structure& st, const std::string& assembly_name,
-                           HowToNameCopiedChain how, const Logger::Callback& logging,
+                           HowToNameCopiedChain how, const Logger& logging,
                            bool keep_spacegroup, double merge_dist) {
   const Assembly* assembly = st.find_assembly(assembly_name);
   std::unique_ptr<Assembly> p1_assembly;
@@ -311,7 +311,7 @@ void transform_to_assembly(Structure& st, const std::string& assembly_name,
   mapping.how = how;
   AssemblyMapping* mapping_ptr = &mapping;
   for (Model& model : st.models) {
-    model = make_assembly_(*assembly, model, how, Logger{logging}, mapping_ptr);
+    model = make_assembly_(*assembly, model, how, logging, mapping_ptr);
     mapping_ptr = nullptr;  // AssemblyMapping is based only on the first model
   }
   finalize_expansion(st, mapping, merge_dist, false);

src/monlib.cpp

@@ -530,7 +530,7 @@ void MonLib::read_monomer_cif(const std::string& path_) {
 
 bool MonLib::read_monomer_lib(const std::string& monomer_dir_,
                               const std::vector<std::string>& resnames,
-                              const Logger::Callback& logging) {
+                              const Logger& logger) {
   if (monomer_dir_.empty())
     fail("read_monomer_lib: monomer_dir not specified.");
   set_monomer_dir(monomer_dir_);
@@ -544,7 +544,6 @@ bool MonLib::read_monomer_lib(const std::string& monomer_dir_,
   ener_lib.read(read_cif_gz(monomer_dir + "ener_lib.cif"));
 
   bool ok = true;
-  Logger logger{logging};
   for (const std::string& name : resnames) {
     if (monomers.find(name) != monomers.end())
       continue;
@@ -598,7 +597,7 @@ double MonLib::find_ideal_distance(const const_CRA& cra1, const const_CRA& cra2)
   return r[0] + r[1];
 }
 
-void MonLib::update_old_atom_names(Structure& st, const Logger::Callback& logging) const {
+void MonLib::update_old_atom_names(Structure& st, const Logger& logger) const {
   for (const auto& it : monomers)
     // monomers should have only monomers needed for this structure.
     // Few of them (usually none or one) have old names defined.
@@ -618,7 +617,6 @@ void MonLib::update_old_atom_names(Structure& st, const Logger::Callback& loggin
               }
             }
       if (old_vs_new > 0) {
-        Logger logger{logging};
         std::string msg = cat("Updating atom names in ", resname, ':');
         std::map<std::string, std::string> mapping;
         for (const ChemComp::Atom& a : cc.atoms)

src/topo.cpp

@@ -899,13 +899,13 @@ void add_hydrogens_without_positions(Topo::ResInfo& ri, const NeighMap& neighbor
 
 NeighMap prepare_neighbor_altlocs(Topo& topo, const MonLib& monlib) {
   // disable warnings here, so they are not printed twice
-  topo.logger.suspended = true;
+  topo.logger.suspend();
   // Prepare bonds. Fills topo.bonds, monomer_rules/link_rules and rt_storage,
   // but they are all reset when apply_all_restraints() is called again.
   topo.only_bonds = true;
   topo.apply_all_restraints(monlib);
   topo.only_bonds = false;
-  topo.logger.suspended = false; // re-enable warnings
+  topo.logger.resume(); // re-enable warnings
   NeighMap neighbors;
   for (const Topo::Bond& bond : topo.bonds) {
     const Atom* a1 = bond.atoms[0];
@@ -923,9 +923,9 @@ NeighMap prepare_neighbor_altlocs(Topo& topo, const MonLib& monlib) {
 std::unique_ptr<Topo>
 prepare_topology(Structure& st, MonLib& monlib, size_t model_index,
                  HydrogenChange h_change, bool reorder,
-                 const Logger::Callback& callback, bool ignore_unknown_links, bool use_cispeps) {
+                 const Logger& logger, bool ignore_unknown_links, bool use_cispeps) {
   std::unique_ptr<Topo> topo(new Topo);
-  topo->logger.callback = callback;
+  topo->logger = logger;
   if (model_index >= st.models.size())
     fail("no such model index: " + std::to_string(model_index));
   Model& model = st.models[model_index];
@@ -1055,11 +1055,11 @@ prepare_topology(Structure& st, MonLib& monlib, size_t model_index,
         vector_remove_if(res.atoms, [](Atom& a) { return a.is_hydrogen() && a.occ == 0; });
 
     // disable warnings here, so they are not printed twice
-    topo->logger.suspended = true;
+    topo->logger.suspend();
     // re-set restraints and indices
     topo->apply_all_restraints(monlib);
     topo->create_indices();
-    topo->logger.suspended = false;
+    topo->logger.resume();
   }
 
   assign_serial_numbers(model);